-
Notifications
You must be signed in to change notification settings - Fork 3
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
Utils compare #43
Utils compare #43
Conversation
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
==========================================
+ Coverage 99.04% 99.15% +0.11%
==========================================
Files 14 14
Lines 522 594 +72
Branches 93 111 +18
==========================================
+ Hits 517 589 +72
Misses 2 2
Partials 3 3 Continue to review full report at Codecov.
|
To explain this pull request - this is a selection of useful functions that were either used to investigate the data or will be used in future pull requests. They don't have an obvious application within the existing codebase but notebooks demonstrating their use will come in the next few PRs. |
CHANGELOG.rst
Outdated
@@ -3,6 +3,7 @@ Changelog | |||
|
|||
master | |||
------ | |||
- (`#44 <https://github.com/znicholls/silicone/pull/44>`_) Implemented new util functions for downloading data, unit conversion and data checking. |
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.
Should this not be #43 ?
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.
yep
src/silicone/utils.py
Outdated
df: :obj:`pyam.IamDataFrame` | ||
The input dataframe. | ||
|
||
to_split : str |
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.
if the other argument is called 'components', perhaps this could be called 'aggregate' (as in 'basket').
Now the naming is a bit inconsistent/not intuitive.
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.
ok, changed
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 is one comment about something that is not clear to me (maybe you can change the use/names there, or explain it with a comment in the code), the rest looks good.
def _adjust_time_style_to_match(self, in_df, target_df): | ||
if in_df.time_col != target_df.time_col: | ||
in_df = in_df.timeseries() | ||
if target_df.time_col == "time": | ||
target_df_year_map = {v.year: v for v in target_df.timeseries().columns} | ||
in_df.columns = in_df.columns.map( | ||
lambda x: target_df_year_map[x] | ||
if x in target_df_year_map | ||
else dt.datetime(x, 1, 1) | ||
) | ||
else: | ||
in_df.columns = in_df.columns.map(lambda x: x.year) | ||
return IamDataFrame(in_df) | ||
|
||
return in_df | ||
return _adjust_time_style_to_match(in_df, target_df) |
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.
this implementation is a bit confusing to me.
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.
Basically I wanted to use this function outside of test/crunchers, so I made it general and then put a version here so I don't have to import it everywhere
Pull request
Please confirm that this pull request has done the following:
CHANGELOG.rst
added