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

Include meta in new IamDataFrames returned by aggregation functions #416

Merged
merged 18 commits into from Jul 27, 2020

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Jul 16, 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 ensures that all aggregation functions include the meta table of the original IamDataFrame when returning a new instance (append=False). As part of the implementation, all test dataframes now have one or two meta column, and the merging of meta tables was moved from the append() function to an own function in utils to be reused.

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #416 into master will increase coverage by 0.02%.
The diff coverage is 97.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   93.43%   93.45%   +0.02%     
==========================================
  Files          35       35              
  Lines        4189     4220      +31     
==========================================
+ Hits         3914     3944      +30     
- Misses        275      276       +1     
Impacted Files Coverage Δ
tests/conftest.py 97.67% <93.75%> (+0.34%) ⬆️
pyam/core.py 92.04% <94.11%> (-0.20%) ⬇️
pyam/utils.py 91.98% <100.00%> (+0.47%) ⬆️
tests/test_feature_aggregate.py 100.00% <100.00%> (ø)
tests/test_feature_append_rename.py 100.00% <100.00%> (ø)
tests/test_iiasa.py 91.59% <100.00%> (-0.08%) ⬇️
tests/test_io.py 100.00% <100.00%> (ø)
tests/test_utils.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 ada621f...bdb3461. Read the comment docs.

Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

All looks fine. The one thing I couldn't see was an explicit test for the behaviour you've changed? Maybe I just missed it though.

pyam/core.py Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member Author

Thanks for the review @znicholls

The one thing I couldn't see was an explicit test for the behaviour you've changed?

The new behavior is tested by adding meta-columns in the test dataframes provided in conftest.py, so the tests check that the new IamDataFrame instances returned by aggregation functions also have these meta indicators (via assert_iamframe_equal()). You could test the test behavior by only extracting the changes to the conftest.py and the test files, but not core.py and utils.py - that should fail.

Anyway, I added an explicit test for merge_meta() to make sure that the precedence of left over right is not accidentally changed in the future.

tests/test_utils.py Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@danielhuppmann danielhuppmann merged commit 337b168 into IAMconsortium:master Jul 27, 2020
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

3 participants