-
Notifications
You must be signed in to change notification settings - Fork 119
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
extend IamDataFrame to use extra data columns and sub-annual time #132
Conversation
Help requested@gidden and @znicholls, what would be an elegant way for running each test three times, once with the existing examples, once with a "subannual" version ( |
revised as suggested by @znicholls to cover initialising an |
Help requested@znicholls, can you implement an appropriate equivalent of the |
1878256
to
e340236
Compare
@danielhuppmann I've got a bit confused here. Do you still need my help with the filter function? I'm guessing we need more tests first as they're currently all passing? |
@znicholls, yes, I still need your help with a filter function for the Re the tests, only a few tests currently check for multiple time formats (all those using Any other column (string that can't be cast to The connection to the IIASA-db's are refactored so that they work with the new implementation, including the tutorial. @znicholls and @gidden, I suggest that you review and merge unless you see major problems. The existing use cases work as expected for the |
@znicholls, I removed the test for extra columns in |
Yep i get that. But if we mark it as xfail, the testsuite will still pass
and we have an easy reminder of what we need to think about. If we just
remove it, finding it again is trickier. The difference is marginal so no
big deal either way.
…On Thu, 29 Nov 2018 at 9:13 pm, Daniel Huppmann ***@***.***> wrote:
@znicholls <https://github.com/znicholls>, I removed the test for extra
columns in meta, because this feature is not yet supported and we need to
discuss this a bit further...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#132 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWh-m0zvbH_fWvi-uJzGOFJ8WrmKB2g5ks5uz919gaJpZM4YLQmp>
.
|
@znicholls , the datetime tests seem to have problems with python 2, can you take a look? |
@znicholls, re having a failing test for
I'd be ok to leave it in if I was convinced that this is the ideal implementation for the long term. But I'd rather have some more discussions on that first... |
cool, will take a look at Python2 stuff now |
@gidden and @znicholls, this should be good to go now... Follow-up issues that we should discuss after this PR is merged:
|
Looks good to me.
Can we split these out into issues? You can assign me to the datetime feature tutorial. |
@znicholls, yes, I meant that we would add these issues after the merge - just thought that it would be clearer what still needs to be done for the larger picture when I spell it out here. Thanks for volunteering! |
aha very good, one step ahead of me! |
* Add test of extra col init behaviour * Add failing tests of time filtering * Setup time filtering tests * Pass test filter year * Redo tests of time filtering and include super messy first steps towards implementation * Fill out tests and reset core * Finish implementation of time filtering, cleaning up needed * Refactor core so apply filters can use self.time_col
as suggested by @znicholls
another rebase after merge of #162 to try to appease stickler (which should have been appeased already) |
closing in favour of #167 |
Please confirm that this PR has done the following:
Description of PR
This PR extends an
IamDataFrame
to have extra (custom) columns in thedata
data frame, and to distinguish betweensubannual
(as string)pd.to_datetime()
The "type" of time representation is stored in
self.time_col
, the additional custom columns inself.data
inself.extra_cols
.One quickfix required to get stuff working:
read_files()
is downgraded to take only one file, not a list of files. Because this would require checking that all imported data has the same time format and same extra columns (or add them on the fly).ToDo:
"year"
toself.time_col
for basic functionality (e.g., intimeseries()
)*_IDX
usage in the package to replace"year"
byself.time_col
and useself.extra_cols
append()
(see issue above withread_files()
)read_files()
Related issue and discussion
Closes #123