IAMconsortium / pyam

Analysis & visualization of energy & climate scenarios
https://pyam-iamc.readthedocs.io/
Apache License 2.0
226 stars 118 forks source link

Add initial IamSlice suggestion #637

Closed coroa closed 2 years ago

coroa commented 2 years ago

This would be a possible way to add an IamSlice feature proposed in #630 .

Please confirm that this PR has done the following:

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

Please describe the changes introduced by this PR.

codecov[bot] commented 2 years ago

Codecov Report

Merging #637 (0b7993b) into main (4741cd8) will decrease coverage by 0.1%. The diff coverage is 83.0%.

@@           Coverage Diff           @@
##            main    #637     +/-   ##
=======================================
- Coverage   94.5%   94.4%   -0.2%     
=======================================
  Files         57      58      +1     
  Lines       5662    5704     +42     
=======================================
+ Hits        5356    5388     +32     
- Misses       306     316     +10     
Impacted Files Coverage Δ
pyam/core.py 93.7% <76.7%> (-0.9%) :arrow_down:
tests/conftest.py 100.0% <100.0%> (ø)
tests/test_filter.py 100.0% <100.0%> (ø)
tests/test_slice.py 100.0% <100.0%> (ø)
tests/test_time.py 100.0% <100.0%> (ø)

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 4741cd8...0b7993b. Read the comment docs.

danielhuppmann commented 2 years ago

I don't feel sufficiently qualified to do a proper review of the implementation, so I wrote a few tests and made some minor modifications - see #642 and #643

coroa commented 2 years ago

Hmm ... that's a pickle. Not sure who could do a review. For me this was basically just trying to get something out to start a discussion.

Undesirable properties (i am aware of):

Other than that i am pretty fine with the result. How far is test-coverage, when your PRs are merged in?

danielhuppmann commented 2 years ago
  • __getitem__ looses meta. it might need a couple of extra postprocessing lines, similar to what is currently in filter (maybe that should be factored out into a utility function). But we could also decide to post-pone the getitem implementation.
  • The slice does not have the same dimension categorisation information as the full dataframe. ie its info can only talk broadly about dimensions, instead of differentiating between meta, timeseries and others.

I don't think that this is a problem. I'd even remove the __repr__ and info methods in a first implementation.

danielhuppmann commented 2 years ago

closing in favor of #657