-
Notifications
You must be signed in to change notification settings - Fork 127
Add a function to initialize an IamDataFrame form an ixmp4 Run #938
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 function to initialize an IamDataFrame form an ixmp4 Run #938
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #938 +/- ##
=======================================
- Coverage 94.8% 94.8% -0.1%
=======================================
Files 68 68
Lines 6458 6474 +16
=======================================
+ Hits 6128 6143 +15
- Misses 330 331 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phackstock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Two small questions below but good to be merged either way.
| year : int or list of int, optional | ||
| Filter timeseries data by time domain. | ||
| """ | ||
| from pyam import IamDataFrame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a special reason to not put the import at the top of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s a circular dependency. The IamDataFrame in core.py imports the function for writing to an ixmp4 Platform, but the functions read_ixmp4 and read_ixmp4_runshould be exposed as top-level functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, maybe there's a good way to rearrange some code to get rid of the circular dependency but that's a job for another day.
| from pyam import IamDataFrame | ||
|
|
||
| if year is not None: | ||
| raise NotImplementedError("Filter by 'year' not implemented in ixmp4.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a feature that will be implemented at some point? Otherwise you could also leave year out as one of the input options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s on my to-do list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah alright, python should have a NotImplementedYetError or an IllGetToItError.
e79e1f3 to
c4648b7
Compare
|
I started working on adding this to the documentation, but found some larger issues, so I will do a follow-up PR with a doc-restructuring. |
|
Refactor to |
Please confirm that this PR has done the following:
Name of contributors Added to AUTHORS.rstDescription of PR
This PR adds a function to initialize an IamDataFrame directly from an ixmp4.Run, to enable workflows like
This is an alternative to the currently possible approach