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

Feature/recursive aggregation #405

Merged

Conversation

tburandt
Copy link
Contributor

@tburandt tburandt commented Jun 26, 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 the functionality to recursively aggregating variables. Based on #385

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

Note that tests are failing because the IIASA infrastructure is unavailable today...

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

thanks @tburandt! By the way, you could have simply accepted the suggestion directly on this page, no need (any more) to copy-paste it into your editor and then commit...

Can you click "resolve" on all @stickler-ci comments that you cleaned up, please?

@tburandt
Copy link
Contributor Author

thanks @tburandt! By the way, you could have simply accepted the suggestion directly on this page, no need (any more) to copy-paste it into your editor and then commit...

Can you click "resolve" on all @stickler-ci comments that you cleaned up, please?

Ha, ok, I wasn't actually aware of that (obviously), now I feel a bit ashamed :D

pyam/_aggregate.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
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.

a few suggestions for improvements...

pyam/core.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
tburandt and others added 2 commits June 30, 2020 11:56
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
pyam/core.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
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 @tburandt, really nice addition to the feature list! just added a few suggestion for clean-up and adding docstrings, then good to be merged...

One question: when you use this with realistically-sized scenario data, is run-time an issue? the append() function is quite costly (because it checks that there are no duplicate rows in data)... if yes, we may want to consider a refactoring for speed (could also be implemented later)...

pyam/core.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Show resolved Hide resolved
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
@tburandt
Copy link
Contributor Author

tburandt commented Jul 2, 2020

Thanks @danielhuppmann for all your support in this PR :)

I will quickly test the feature with a larger dataset

@tburandt
Copy link
Contributor Author

tburandt commented Jul 2, 2020

@danielhuppmann
With the current GENeSYS-MOD dataset, the time for the recursive feature is almost identical to the manual aggregation of the subcategories. After several runs with different scenarios, I couldn't observe any significant difference. However, in my original script, I also used the append-parameter for creating the final dataset for all existing subcategories.

@danielhuppmann
Copy link
Member

took the liberty to resolve the merge conflict...

@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #405 into master will increase coverage by 0.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   93.25%   93.26%   +0.01%     
==========================================
  Files          35       35              
  Lines        4090     4129      +39     
==========================================
+ Hits         3814     3851      +37     
- Misses        276      278       +2     
Impacted Files Coverage Δ
pyam/core.py 92.14% <71.42%> (-0.22%) ⬇️
pyam/_aggregate.py 96.52% <100.00%> (+0.68%) ⬆️
tests/test_feature_aggregate.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 0d5bc4a...0ebda62. Read the comment docs.

@danielhuppmann
Copy link
Member

thank you @tburandt! let's keep in mind that this feature could probably be faster - so let's revisit it and refactor the code when aggregating the results takes longer than running the model...

@danielhuppmann danielhuppmann merged commit 68856a3 into IAMconsortium:master Jul 2, 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