Day of week effect #89
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files📢 Thoughts on this report? Let us know! |
zsusswein
force-pushed
the
day-of-week
branch
from
dd0a2da to
753367f
Compare
zsusswein
force-pushed
the
day-of-week
branch
from
753367f to
b5a75b7
Compare
Gratia development version works fine, but the current CRAN version has a bug in its factor handling. When passing (known) factor levels, it errors out when constructing the dataset in `data_slic()`. This error wasn't showing up in the tests because I had installed the development version from Github. But it was getting caught on CI and in the vignette because those were installing the latest CRAN version. I had to implement an imperfect workaround here. It's messy because each individual factor level is "far" from the data in mgcv's definition -- at least far enough to throw a warning. It would be pretty labor intensive to reconstruct the associated levels because we explicitly set them to zero with `exclude`. I don't want to do all the work of reconstructing the matching levels because to then immediately toss them. So instead, I've suppressed the warning -- an imperfect but "good enoough" solution...
seabbs
reviewed
Dec 3, 2024
seabbs
reviewed
Dec 3, 2024
seabbs
reviewed
Dec 3, 2024
seabbs
reviewed
Dec 3, 2024
seabbs
reviewed
Dec 3, 2024
seabbs
reviewed
Dec 3, 2024
seabbs
approved these changes
Dec 3, 2024
There was a problem hiding this comment.
This looks correct to me though is convoluted as you say. I am not sure there is much of a way around that.
I have flagged some things for new issues and a few points of complexity we are starting to see from the vector based interface (IMO). The only actionable thing to check is whether you can get away with passing reference_date as the default to day_of_week and so avoid having an argument with multiple supported types.
Co-authored-by: Sam Abbott <[email protected]>
kgostic
reviewed
Dec 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements a day of week effect for the model and handling for day of week in predictions. The model will automatically implement a day of week by default, but also allows for a custom vector of levels to be passed to the model or for the day of week estimation to be turned off. The predict method will automatically handle day of week effects where possible and, when not possible, throws informative error messages with changes needed and why.
Note that this implementation took some custom handling logic and fiddling with some internal gratia errors. I think I tested that all pretty carefully and documented it thoroughly, but it took a couple of tries. It wasn't quite as easy I was originally hoping last week.
Closes #87