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 a feature to swap_year_for_time #560

Merged

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Jul 8, 2021

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

This PR introduces the "inverse" of the swap_time_for_year() method. This should facilitate conversion from and to proper datetime timeseries and the "subannual" column currently used in the IIASA database infrastructure.

As shown in the tests, if you have an IamDataFrame with datetime format, then

df.swap_time_for_year(subannual=True).swap_year_for_time()

will yield an identical object.

@danielhuppmann danielhuppmann added the datetime Compatibility with `datetime` time format label Jul 8, 2021
@danielhuppmann danielhuppmann self-assigned this Jul 8, 2021
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #560 (5adccd8) into main (4b90533) will decrease coverage by 0.0%.
The diff coverage is 94.1%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #560     +/-   ##
=======================================
- Coverage   93.6%   93.6%   -0.1%     
=======================================
  Files         50      50             
  Lines       5283    5313     +30     
=======================================
+ Hits        4950    4978     +28     
- Misses       333     335      +2     
Impacted Files Coverage Δ
pyam/time.py 96.0% <90.9%> (-4.0%) ⬇️
pyam/__init__.py 81.4% <100.0%> (+0.7%) ⬆️
pyam/core.py 94.2% <100.0%> (+<0.1%) ⬆️
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 4b90533...5adccd8. Read the comment docs.

@danielhuppmann
Copy link
Member Author

@loeffko @sebastianzwickl @erikfilias, this may be helpful for your ongoing work in the model linkage for openENTRANCE.

@danielhuppmann danielhuppmann marked this pull request as ready for review July 8, 2021 08:12
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.

My one quick thought. Lots of edge cases could be covered here, depends how widely this feature is expected to be used.

"""
return swap_time_for_year(self, subannual=subannual, inplace=inplace)

def swap_year_for_time(self, inplace=False):
"""Convert the `year` and `subannual` dimensions to `time` (as datetime).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth adding that the conversion is done using dateutil.parser.parse on the combination of year and subannual?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, great suggestion - added this to the docstring including intersphinx mapping...

@danielhuppmann
Copy link
Member Author

My one quick thought. Lots of edge cases could be covered here, depends how widely this feature is expected to be used.

The unit tests already cover five basic cases, including the one we are using in the openENTRANCE and ECEMF projects - this is the one where I see this function actually being used.

https://github.com/danielhuppmann/pyam/blob/5adccd81a8416aac3ed185b7bd33a7bb7c0cb515/tests/test_time.py#L55

I don't see any other use cases on the horizon (like using a different column name than "subannual" for the non-year parts, or using several columns) - but no reason why these two "swap" functions couldn't be extended in that direction with keyword arguments...

@danielhuppmann
Copy link
Member Author

Merging as there were no objections or further comments... Thanks @znicholls for the suggestion to improve the docs!

@danielhuppmann danielhuppmann merged commit 7d77473 into IAMconsortium:main Jul 15, 2021
@danielhuppmann danielhuppmann deleted the time/merge_subannual branch July 15, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datetime Compatibility with `datetime` time format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants