-
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
Last quantile #126
Last quantile #126
Conversation
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.
Great to see that you're extending the functionality of silicone - I think it will be very beneficial to have this additional infiller in the package.
I was able to run and it performs as expected - so all good from a functionality point-of-view.
However, I was reflecting a bit on that this 'infiller' is significantly different from the others and i was wondering if this should be reflected in the documentation a bit more.
All other infillers are about adding more variables, where this is about extending a variable temporally. What do you think about making this separation more clear? Would it be conceivable to that in the future more 'temporal-extension' infillers?
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.
My two cents from a very quick read. Looks like awesome functionality, very nice. My only question: why the changes to the quantile walk infilled? As a note, if these changes to the quantile walk are kept then they would be API breaking changes so we'd need to bump the major version on next release. My suggestion would be to only make those changes if they're really needed (they may well be).
Thanks for your reviews guys! I was indeed thinking of making a separate section for this (on the basis that there may be other projector functions upcoming) so if it occurs to other people then I'll make a separate section for them. @znicholls the changes to the quantile walk are simply to extract a part of the code to allow it to be reused - there is no change to the output (you will see I haven't changed the test for it) so it's not API breaking. The next push will feature additional quantile options that are tedious to duplicate, so it's better to have reused code. |
Definitely a good idea to reuse. I've highlighted the API breaking bit, it's pretty minor so not the end of the world. |
OK, I refactored it to separate out time projectors (I decided to make it the name sound less like a sci-fi body implant). @znicholls I don't see any inline comments, should I? |
Looking good to me. I also can't see the 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.
Oops sorry i forgot to hit send
@@ -61,19 +61,15 @@ def derive_relationship(self, variable_follower, variable_leaders): | |||
lead_ts = self._db.filter(variable=variable_leaders).timeseries() | |||
lead_unit = lead_ts.index.get_level_values("unit")[0] | |||
|
|||
def filler(in_iamdf, interpolate=False): |
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.
Removing interpolate
as a keyword argument is minor but nonetheless API breaking (it is true that we never tested it).
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.
Oh, that. Yes, it was never actually attached to anything, so I hope no one used it and we should definitely get rid of it (I continue to think that people should just interpolate everything before they put the data in). We can spike the version number to 2 if you want as a result when we do the next release. Perhaps we should write a helper function that does the interpolation in a sensible way instead.
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.
I think just putting something in the CHANGELOG will suffice (particularly given that we never tested it and probably no-one used it). It just jumped out so I thought I should raise.
I continue to think that people should just interpolate everything before they put the data in
We could certainly add that advice in docs and/or add stronger 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.
OK, I'll update the changelog in the next push.
Pull request
Please confirm that this pull request has done the following:
CHANGELOG.rst
addedAdding to CHANGELOG.rst
Please add a single line in the changelog notes similar to one of the following: