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

Region aggregation if region missing #267

Merged
merged 6 commits into from
Oct 17, 2019

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented Sep 20, 2019

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Description in RELEASE_NOTES.md Added

Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)

Please add a single line in the release notes similar to the following:

- (#XX)[http://link-to-pr.com] Added feature which does something

Description of PR

Updates pyam so behaviour of check_aggregate_region is sensible even if the region your checking is missing.

@znicholls
Copy link
Collaborator Author

@danielhuppmann @gidden thoughts would be great here. The new tests currently fail in a cryptic way. The question is, if you call check_aggregate_region and the region you're checking isn't there, should an error be raised, a warning or just nothing and None is returned (cause a region can't be wrong if it's not there...)?

@danielhuppmann
Copy link
Member

From a quick look, it seems that the command tries to return the object as a pivoted pd.DataFrame (via timeseries()), which doesn't work if the dataframe is empty.

On the question "how do we expect this to behave", I would follow the behaviour in aggregate_region() (log message and empty return):

msg = 'cannot aggregate variable `{}` to `{}` because it does not'\

pyam/core.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 87.86% when pulling 2f335a6 on znicholls:region-aggregation-bug into 127680a on IAMconsortium:master.

@coveralls
Copy link

coveralls commented Sep 22, 2019

Coverage Status

Coverage decreased (-0.2%) to 87.873% when pulling cf09266 on znicholls:region-aggregation-bug into dd7ae0c on IAMconsortium:master.

@znicholls znicholls marked this pull request as ready for review September 22, 2019 22:38
@znicholls
Copy link
Collaborator Author

@danielhuppmann this should be good to go

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

thanks @znicholls for improving the user-friendliness of the region.aggregation-checking function!

@danielhuppmann danielhuppmann merged commit 4ba54fa into IAMconsortium:master Oct 17, 2019
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.

4 participants