Add UnivariateTimeTypeToContinuous transformer to builtins #245
Conversation
src/builtins/Transformers.jl
Outdated
| """ | ||
| UnivariateTimeTypeToContinuous(zero_time=nothing, step=Hour(24)) | ||
|
|
||
| Convert `Date`, `DateTime`, and `Time` vectors to `Float64` by assuming `0.0` corresponds |
There was a problem hiding this comment.
Minor: I suggest "Convert a Date, DateTime, and Time vector to prevent confusion with multi-variate case.
|
Good feedback. I’ll update the PR soon. |
There was a problem hiding this comment.
Okay this is looking pretty solid. Only some minor comments and suggestions for checks to add (bells and whistles!)
Regarding the check in clean! I suggest above: Best to issue a @warn and reset the bad input to something reasonable than to throw an error. Specify in the warning what change is made. See the example at the docs
Re tests, I am sure you can cook up some reasonable tests 😄 Would be good to check:
-
user specified
time_stepworks as well as automatic case -
check your code tweak for time_step = 24hours in
Timesetting works ( 5am + 24hrs = 5am mod 24hrs) -
use
@test_throwsto check incompatible types at time of fit or transform -
use
@test_logsto check any warnings you implement inclean!
|
I added the checks and I pushed the new version to the dev branch of venuur/MLJModels.jl. According to github the changes should show up here (though I don't see them yet). I'll be adding tests later in the week. |
That's weird. This latest commit here is 8881832, which matches the latest commit on your dev branch just now:
Are you sure they are not there? |
|
When I look at the commits tab up top, I see the commits in the attached screenshot. The date doesn’t match what you are showing. |
|
Your latest commit appears there, it seems to me. It's at the bottom. Perhaps what's confusing you is that this tab lists the commits in the opposite order to that in the conversations tab? Sorry I can't be more helpful. |
|
My latest commit on my fork is venuur@3abaa09 That is not the one on this pull request, right? Or am I just reading it all wrong, which is totally possible. |
|
No sorry, I must have been looking in the wrong place. This is quite weird. Going to close and re-open - see if that helps |
|
Nah. Can you add |
|
Looks like it's updated finally. |
|
Looking great! Thanks adding such detailed checks. I think all that's needed now are some tests. |
|
@venuur Be great to have some tests when you get a chance. |
|
Thanks for pinging me. I haven’t forgotten about this, it’s just my workload has been heavier than usual so it may be another week or two before I can wrap this up. I’ll definitely get to it, the timeline is just a bit pushed compared to my original plan. |
|
No worries at all!! |
…els.jl into venuur-add-timetype-continuous
Fix default constructor case of Hour(24) when applied to a Date vector. Hour cannot be added to Date objects, so the step must be converted to Day, but because this is a change to the step hyper parameter it must be adjusted. Unfortunately, this case cannot be distinguished from a user supplied date, so a warning must be shown, which is non-ideal for a common default case. Alternatively we can add an extra internal variable to track whether a user supplied step is given and only warn on that case, but that is extra complexity I am avoiding for this commit.
|
Added tests and fixed a couple corner cases while writing the log tests. There's an annoying issue that the default constructor uses a step of Hour(24), but when applied to a Date, we need to convert to Day(1), which is fine, but since the constructor doesn't distinguish a user supplied step versus the default, a warning message is printed. I would rather add some extra checks to avoid the message if it's the default value, but I don't have the time right now to figure out the right logic. Excepting the somewhat unnecessary log message, the current version should be complete. Please let me know if I'm missing anything. |
|
Should be all good for your next review when you get a chance @ablaom |
There was a problem hiding this comment.
This is terrific, many thanks. Two small details remain. Great if you could put these in:
-
add "summary" doc-string
UNIVARIATE_TIME_TYPE_TO_CONTINUOUSlike the others appearing at the top of transformers.jl. -
add a metadata entry, which can go at the very end of transformers.jl. It should look like this:
metadata_model(UnivariateTimeTypeToContinuous,
input = AbstractVector{<:ScientificTypes.ScientificTimeType},
output = AbstractVector{Continuous},
weights = false,
descr = UNIVARIATE_TIME_TYPE_TO_CONTINUOUS,
path = "MLJModels.UnivariateTimeTypeToContinuous")
|
And, if you haven't noticed, there's a failure for julia 1.0. Here's an excerpt: |
|
Sounds good. I’ll make the changes you suggested and look into the Julia 1.0 error. |
|
Okay, working on the conflicts and adding the extras referred to above ... |
|
Thanks for following up. And sorry for disappearing. As you may guess, I haven’t had time and won’t have time for a while to work on this. I’m still happy to comment here where I can. |
Implements univariate case of TimeType transformer discussed in #234
I would appreciate feedback, especially recommendations on how to add tests.