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

Scenarios module #165

Merged
merged 21 commits into from May 24, 2019
Merged

Scenarios module #165

merged 21 commits into from May 24, 2019

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented May 22, 2019

Adds scenario module to OpenSCM. This gives us a bunch of scenarios with which we can test models and also makes it easy for first-time users to get a feel for how OpenSCM works without having to wrangle scenario data to get started.

Also includes a minor bugfix in ScmDataframe's interpolation method.

  • Implementation finished
  • Tests added
  • Documentation added
  • Example added (in the documentation, to an existing notebook, or in a new notebook)
  • Description in CHANGELOG.rst added (single line such as: (`#XX <https://github.com/openclimatedata/openscm/pull/XX>`_) Added feature which does something)

Copy link
Member

@swillner swillner left a comment

Choose a reason for hiding this comment

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

Nice!
Just a few requests:

  • Can you please clean the notebooks? ;) (we really need a check for that ;) )
  • Can we have these CSVs as tables including only columns "Variable","Unit","Year", please?
  • 20thcentury_emissions or historical_emissions (you also have some from the 18th century in there)?
  • The historical emissions don't seem to be in the scenario module
  • What about lazy-loading them?

@znicholls
Copy link
Collaborator Author

thanks @swillner! I think I've addressed all the comments now so ready for round 2

@znicholls znicholls requested a review from swillner May 23, 2019 06:21
@znicholls
Copy link
Collaborator Author

we might want to re-write the timeseries notebook to make it more obvious that OpenSCM is super to easy to use, putting the details later in the notebook for those who are interested.

@swillner
Copy link
Member

I would very much like the timeseries conversion notebook stick to the internal procedure and thus not use normally exposed interfaces... Probably we should have a thorough revision of all the notebooks once the package itself is in a releasable state.

So without those changes I think we are good to go. Maybe rename the column to "ParameterType" to be consistent with the (apparent) camel case names of the other columns (or use snake_case throughout)?

@znicholls
Copy link
Collaborator Author

znicholls commented May 23, 2019 via email

@swillner
Copy link
Member

Yes, it's about the internals, right? Anyway, let's leave the notebooks as they are right now and properly clean them once have a release candidate.

can you then also make the other columns snake case, please? ;)

@znicholls
Copy link
Collaborator Author

Yes, it's about the internals, right?

I thought it was about the fact that there are two different conventions with respect to timeseries and that they have to be handled differently.

Anyway, let's leave the notebooks as they are right now and properly clean them once have a release candidate.

Ok I'll just add a note in the top saying they're likely to be re-written once we have a release and put some suggestions about how re could re-write

@swillner
Copy link
Member

Cool, just give me a ping and I'll merge ;)

@znicholls
Copy link
Collaborator Author

@swillner ping. A lot of the notebook changes are unavoidable cause of the case changes and the fact we can use scenarios now rather than downloading from iiasa in examples

Copy link
Member

@swillner swillner left a comment

Choose a reason for hiding this comment

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

Nice

@swillner swillner merged commit 8eb6f04 into master May 24, 2019
@swillner swillner deleted the scenarios-module branch May 24, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants