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

Add a feature to pass an explicit weight dataframe to downscale_region() #411

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Jun 29, 2020

Please confirm that this PR has done the following:

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

Description of PR

This PR adds an option to pass an explicit weight-dataframe with region (and possibly others) as index dimension and the time domain as columns. Implementation based on many discussions with @maartenbrinkering on his specific use case.

@danielhuppmann danielhuppmann added the downscaling Features related to (regional, sectoral) downscaling of data label Jun 29, 2020
pyam/core.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
tests/test_feature_downscale.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #411 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   93.26%   93.33%   +0.06%     
==========================================
  Files          35       35              
  Lines        4129     4169      +40     
==========================================
+ Hits         3851     3891      +40     
  Misses        278      278              
Impacted Files Coverage Δ
pyam/core.py 92.24% <100.00%> (+0.10%) ⬆️
tests/test_feature_downscale.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 911dcd4...1f34641. Read the comment docs.

@danielhuppmann
Copy link
Member Author

@fabiosferra, maybe also of interest to you?

Copy link
Collaborator

@maartenbrinkerink maartenbrinkerink left a comment

Choose a reason for hiding this comment

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

Approved. Based on the tests we've done the feature works as intended.

@l-welder
Copy link
Collaborator

l-welder commented Jul 3, 2020

Tested the new features locally and everything seems to be working as intended. Looking forward to add to this (=

Copy link
Collaborator

@l-welder l-welder left a comment

Choose a reason for hiding this comment

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

looks great!

on the risk of appearing meticulous: passing the default values of optional arguments is not 100% consistent in the doc string (e.g. region lost "World")... but I see that is also the case for other docstrings and not really of importance.... but I had to find something (=

@danielhuppmann
Copy link
Member Author

Thanks @l-welder for the review and noticing the change in the docs. We actually started out specifying the default in the docstrings, but (if I recall correctly) @khaeru pointed out that the NumPy-style docstrings convention is to just write "optional", because the default value is anyway obvious from the function signature. So I'm changing this whenever I stumble over the legacy docstrings...

@danielhuppmann danielhuppmann merged commit bcf8293 into IAMconsortium:master Jul 8, 2020
@danielhuppmann danielhuppmann deleted the feature/downscale-region-weights branch July 8, 2020 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
downscaling Features related to (regional, sectoral) downscaling of data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants