-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
kemccusker
commented
Sep 27, 2022
•
edited by brews
Loading
edited by brews
- Fixes "global_cons" arg unused in dscim.utils.menu_runs.run_rff() #39
- Fixes convert_old_to_newformat_AR()'s "pulseyrs" arg has a mutable default #31
@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. |
There was a problem hiding this 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.
Should note this PR has breaking changes to That's fine but worth mentioning in case someone wants to know why their code no longer works. |
Thanks for listing the breaking changes, @brews. @davidrzhdu is handling the I want to confirm from @JMGilbert that we no longer need |
I am pretty sure that we no longer use this argument or need it at all. |
For future reference:
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. |
I'm going to update the CHANGELOG manually after this merges as time is tight on this. |