Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue fixes (#39 and #31) #51

Merged
merged 3 commits into from
Sep 29, 2022
Merged

Issue fixes (#39 and #31) #51

merged 3 commits into from
Sep 29, 2022

Conversation

kemccusker
Copy link
Member

@kemccusker kemccusker commented Sep 27, 2022

@brews
Copy link
Member

brews commented Sep 29, 2022

@kemccusker This is marked as "Draft" in the title. Is this ready to review and merge or is another component missing from this PR?

@kemccusker kemccusker changed the title Draft: Issue fixes Issue fixes (#39 and #31) Sep 29, 2022
@kemccusker
Copy link
Member Author

kemccusker commented Sep 29, 2022

@kemccusker This is marked as "Draft" in the title. Is this ready to review and merge or is another component missing from this PR?

Oops sorry @brews, removed the Draft. It's ready for review.

Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think this can merge whenever you're ready, @kemccusker.

@brews
Copy link
Member

brews commented Sep 29, 2022

Should note this PR has breaking changes to dscim.preprocessing.climate.reformat.convert_old_to_newformat_AR() and dscim.utils.menu_runs.run_rff() because it drops the pulseyrs and global_cons arguments.

That's fine but worth mentioning in case someone wants to know why their code no longer works.

@kemccusker
Copy link
Member Author

Thanks for listing the breaking changes, @brews. @davidrzhdu is handling the pulseyrs args in other functions.

I want to confirm from @JMGilbert that we no longer need global_cons for any reason -- it is unused here, but used to be a flag that would force the use of "global consumption" for discounting of domestic damages. I believe (and @JMGilbert can confirm) that we have updated the EconVars and configs to specify explicitly which consumption file to use for discounting and no longer need the flag.

@JMGilbert
Copy link
Contributor

Thanks for listing the breaking changes, @brews. @davidrzhdu is handling the pulseyrs args in other functions.

I want to confirm from @JMGilbert that we no longer need global_cons for any reason -- it is unused here, but used to be a flag that would force the use of "global consumption" for discounting of domestic damages. I believe (and @JMGilbert can confirm) that we have updated the EconVars and configs to specify explicitly which consumption file to use for discounting and no longer need the flag.

I am pretty sure that we no longer use this argument or need it at all.

@JMGilbert
Copy link
Contributor

For future reference:
global_cons was formerly the argument used to save out global_consumption_no_pulse.nc4 files. This has been deprecated because of the @save decorator implemented into dscim which allows for saving through the config:

global_parameters:
  save_files:
      - global_consumption_no_pulse

When calculating country specific SCGHGs, discounting using only the future socioeconomics of that country would be improper. To obtain the proper SCGHG, calculate discount factors using global consumption no pulse, then discount the marginal damages from that country and sum across years.

@kemccusker kemccusker merged commit 440ee27 into main Sep 29, 2022
@brews brews deleted the issue_fixes branch September 29, 2022 21:44
@brews
Copy link
Member

brews commented Sep 29, 2022

I'm going to update the CHANGELOG manually after this merges as time is tight on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants