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 initial IamSlice suggestion #637

Closed
wants to merge 6 commits into from
Closed

Add initial IamSlice suggestion #637

wants to merge 6 commits into from

Conversation

coroa
Copy link
Collaborator

@coroa coroa commented Mar 3, 2022

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

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • 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

Please describe the changes introduced by this PR.

coroa added 2 commits March 3, 2022 17:26
Depending on what is filtered it returns a boolean mask as a pd.Series or as a numpy array :/
@codecov
Copy link

codecov bot commented Mar 3, 2022

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%) ⬇️
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
Copy link
Member

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
Copy link
Collaborator Author

coroa commented Mar 13, 2022

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):

  • __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.

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

@danielhuppmann
Copy link
Member

  • __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 and others added 2 commits March 15, 2022 13:17
* Add tests and set correct return type for `time`

* Implement suggestion by @coroa

* Add `name` to pd.Index returned by `time`

* Deactivate unfccc-test
* Add a proper length attribute

* Use pandas native method

Co-authored-by: Jonas Hörsch <coroa@posteo.de>
coroa and others added 2 commits March 15, 2022 20:42
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
@danielhuppmann danielhuppmann mentioned this pull request May 6, 2022
3 tasks
@danielhuppmann
Copy link
Member

closing in favor of #657

@danielhuppmann danielhuppmann deleted the add-iamslice branch September 1, 2022 11:53
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.

2 participants