Skip to content
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

Create custom [.forecast() method #884

Merged
merged 13 commits into from
Aug 19, 2024
Merged

Create custom [.forecast() method #884

merged 13 commits into from
Aug 19, 2024

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Aug 6, 2024

This is a first draft for #816

Todo:

  • Add custom head() and tail() methods to avoid unnecessary warnings
  • Add setter methods
  • Ensure data.table features are conserved in the new method

Open questions / discussion points:

  • How to reduce duplication in ensuring we manipulate the right class? Maybe a new unclass argument in clean_forecast()?
  • Double check I'm using data.table correctly and not changing in place when not appropriate

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great so far. data.table usage looks sensible though I think this now creates a copy where it didn't before (i.e every time as.data.table is called). I don't think this is a major issue though.

@nikosbosse
Copy link
Contributor

as.data.table() does make a copy by default

@nikosbosse
Copy link
Contributor

I merged new features into main and then thought it would be a good idea to fix the ensuing merge conflicts here. Maybe I should not have done that, apologies for interfering with your code.

This created a test failure due to the fact that the functions for nominal forecasts I implemented modified forecasts objects in a way that triggered your checks.

I made a PR targeting this PR to fix these issues: #892

@Bisaloo Bisaloo marked this pull request as ready for review August 13, 2024 09:20
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I earnt some neat stuff from this PR. As far as I am concerned, this is good to go but think we should wait for @nikosbosse to review and confirm.

@nikosbosse
Copy link
Contributor

This is great, thanks so much!

f <- as_forecast_quantile(example_quantile)
f <- f |> 
  dplyr::select(-observed)
f

Just noting that we wouldn't get any warnings when people use dplyr. But I assume that is expected/intended, right?

@nikosbosse
Copy link
Contributor

It feels like replacing the as.data.table() calls eventually would be nice, but we can also make this an issue and address it later.

@seabbs seabbs merged commit c20e2aa into main Aug 19, 2024
9 checks passed
@seabbs seabbs deleted the instant-invalidation branch August 19, 2024 20:47
@seabbs
Copy link
Contributor

seabbs commented Aug 19, 2024

oo sorry Nikos I read this first as a approval and not a comment and so hit merge....

I agree as should look at the data.table issue separately.

@nikosbosse
Copy link
Contributor

No worries, I think it's fine to merge. @Bisaloo thanks a lot, this was a really cool PR!
Do you think there is something clever we can do with dplyr as well? I remember looking into what they use under the hood for mutate at some point. But maybe we also don't want to get too far down the rabbit hole...

Bisaloo added a commit that referenced this pull request Aug 27, 2024
This ensures that object manipulated by [ via dplyr function are still validated.
See the report in #884 (comment).
Bisaloo added a commit that referenced this pull request Aug 27, 2024
This ensures that object manipulated by [ via dplyr function are still validated.
See the report in #884 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants