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

Do not sort on initialization or append #893

Merged
merged 14 commits into from
Dec 16, 2024

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Dec 9, 2024

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

Description of PR

So far, pyam sorted the timeseries data upon initialization and when appending data (including aggregation, and binary operations). This is probably a performance drag in scenario-processing where multiple aggregation steps are performed iteratively.

Also, there was an issue where the result of timeseries() did to always show the years in order.

This PR is the first of a sequence of planned improvements, removing the automated sorting and ensuring that timeseries in wide format are always returned in the right order.

As a next step, I will implement a sort_data() method (in case this is needed), remove automated sorting from other methods (e.g., rename()), and make sure that compare() and check_*() methods word on unsorted data.

@danielhuppmann danielhuppmann self-assigned this Dec 9, 2024
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.0%. Comparing base (ddbb88e) to head (3902dcf).
Report is 39 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff           @@
##            main    #893    +/-   ##
======================================
  Coverage   95.0%   95.0%            
======================================
  Files         64      65     +1     
  Lines       6134    6319   +185     
======================================
+ Hits        5828    6006   +178     
- Misses       306     313     +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@byersiiasa
Copy link
Collaborator

This is a very welcome improvement!

Not using pyam much these days and I couldn't see the functionality of sort_data() quickly - but, if not included already, being able to choose the ordering of whether sort by model/scenario/variable could be useful downstream when plotting. IIRC there are now options to provide some manual sort argument when plotting, but this could be an alternative, or useful for other purposes!

@danielhuppmann
Copy link
Member Author

sort by model/scenario/variable could be useful downstream when plotting

Not quite what this PR had in mind, the plotting library already supports an order keyword argument for area and stacked bar charts.

@danielhuppmann danielhuppmann merged commit a08f7c3 into IAMconsortium:main Dec 16, 2024
14 checks passed
@danielhuppmann danielhuppmann deleted the feature/no-sort branch December 16, 2024 04:42
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