Fix bug by allowing emission_scenarios to be None#46
Conversation
| ecs_mask_name=None, | ||
| base_period=(2001, 2010), | ||
| emission_scenarios=None, | ||
| emission_scenarios=(), |
There was a problem hiding this comment.
Just wondering if we want to be setting a default set of scenarios at all (then we wouldn't need to check for if == ()). Or if we should force the user to specify the scenarios, (can still be a default through the configs or something like that).
Perhaps a future adjustment to make.
kemccusker
left a comment
kemccusker
left a comment
There was a problem hiding this comment.
Looks good. Please update the docstring for emission_scenarios, otherwise it looks ready to merge!
brews
left a comment
brews
left a comment
There was a problem hiding this comment.
Hey @JMGilbert. Thanks for looking at this. Interesting bug. This is starting to feel more hacky as I'm not entirely clear on the meaning when emission_scenarios is None. It was kinda unclear to begin with so I don't mean it's not just your changes here. I have an in-line comment on updating the docstr so we at least know what the None means.
| emission_scenarios: list or None, optional | ||
| List of emission scenarios for which SCC will be calculated. Default | ||
| is ["ssp119", "ssp126", "ssp245", "ssp460", "ssp370", "ssp585"]. | ||
| is (), which gets set to ["ssp119", "ssp126", "ssp245", "ssp460", "ssp370", "ssp585"]. |
There was a problem hiding this comment.
@JMGilbert What does "None" actually mean then? This isn't documented anywhere (or it appears unclear).
|
@JMGilbert I'd recommend adding one sentence to |
3 participants
Fix #45 by allowing
emission_scenariosto beNonewithout modifying its value during instantiation. This allows dscim-epa to replicate prior results and should allow for the update_dscim branch indscim-epato be merged.