-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
switch to interval_[start/end] + allow forecast non-zero first step #64
Conversation
interval_start=minutes(nwp_config.interval_start_minutes), | ||
interval_end=minutes(nwp_config.interval_end_minutes), | ||
dropout_timedeltas=minutes(nwp_config.dropout_timedeltas_minutes), | ||
dropout_frac=nwp_config.dropout_fraction, |
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.
Looking at the way sat delay is handled now, do we ever actually use the dropout fraction system for NWPs (or at all)? Because if we always just set it to one delay that is always used, then we can just do it through interval_end as well.
Though to be honest I am a bit on the fence about factoring the delay into interval start/end, I think it can get messy and is very susceptible to human error. I like the way current NWP dropout system lets you say "I want x amount backward and y amount forward, latest you can get me" and it factors in the delay for you. When setting up sat config it used to be "I want an hour of history, but the delay is 30 minutes, so actually I need to request 90 minutes of history" etc, which is bad, and moving to "I want an hour of history with delay 30 min, so I need everything between -90 min and -30 min" is better, but still a long way away from "I want this much, you'll probably need to wait that much for it to come in, go figure it out".
What am I missing?
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.
Looking at the way sat delay is handled now, do we ever actually use the dropout fraction system for NWPs (or at all)? Because if we always just set it to one delay that is always used, then we can just do it through interval_end as well.
You're right we always set the NWP dropout to a constant value. I'm not sure how we would use interval_end
for that though. Currently interval_start/end
define the slice along the step
dimension. t0
and dropout_timedeltas_minutes
define the slice along the init-time
dimension.
I like the way current NWP dropout system lets you say "I want x amount backward and y amount forward, latest you can get me" and it factors in the delay for you.
That hasn't really changed in this PR. We are basically just renaming the history and forecast parameters. So after this PR it is "I want between [t0+x, and t0+y], the latest init-time you can get me". For NWP it does still factor in the delay for you.
I think this PR gives a cleaner way of expressing the time slice. For the manchester prize satellite prediction input I want to slice between t0+15 minutes and t0+3 hours. Under our old system I'd need to say I wanted negative 15 minutes of history which seems clumsy.
When setting up sat config it used to be "I want an hour of history, but the delay is 30 minutes, so actually I need to request 90 minutes of history" etc, which is bad, and moving to "I want an hour of history with delay 30 min, so I need everything between -90 min and -30 min" is better, but still a long way away from "I want this much, you'll probably need to wait that much for it to come in, go figure it out".
Personally I like the explicitness of setting everything relative to t0. To me it better highlights the data we expect to be available in production -> i.e. we can't delete production data until it is more than 90 minutes stale. But I'm open to suggestions for how else we might parameterise this
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.
You're right we always set the NWP dropout to a constant value. I'm not sure how we would use
interval_end
for that though. Currentlyinterval_start/end
define the slice along thestep
dimension.t0
anddropout_timedeltas_minutes
define the slice along theinit-time
dimension.
Yeah good point, my bad!
I think this PR gives a cleaner way of expressing the time slice. For the manchester prize satellite prediction input I want to slice between t0+15 minutes and t0+3 hours. Under our old system I'd need to say I wanted negative 15 minutes of history which seems clumsy
I think this is the use case I was missing, it makes a lot of sense now, thanks!
Not a hill I will die on, but for what it's worth: I think this intervaling is convenient, especially if you want an "unconventional" sat slice (might even enable 1 image of sat history that I previously had to hack around to get with forecast/history, which is great!) but also because for sat data init_time
and step
are the same thing, it kind of mixes the two together if that makes sense? Which is why it doesn't work for NWP delay setting, and why I initially thought it would. In that sense, I would maybe prefer for the delay to still be set separately and functioning the way NWP delay does and not how it was previously, so to be able to set interval (-60, -15) and delay -30 (btw, hot take but delay already implies moving back, so maybe shouldn't be set in negative? Seems kind of counter-intuitive to me. But there's probably a good reason and also, I digress), instead of interval (-90, -45), or how it was previously, history 90 delay 45 under the assumption that the tail will get cut off.
Main reason: I know at least I am extremely prone to human error in config setting and would like as much automation as possible, and also prefer more single-function things and less multi-function things.
I know we've talked a lot about eradicating live sat delay, and maybe this is a bit excessive parametrising for what it does, but to me it seems a bit more straightforward on user end. Also, feel free to tell me what I missed! Fully expect to be wrong on this one, I have way less experience with sat than you do.
Personally I like the explicitness of setting everything relative to t0. To me it better highlights the data we expect to be available in production -> i.e. we can't delete production data until it is more than 90 minutes stale. But I'm open to suggestions for how else we might parameterise this
Here you'd 100% know better than I do! I get that this is a bit of a have your cake and eat it thing and if your prod cake is more useful than my user cake I'm happy for intervaling to cover delay as well.
By the way, thanks a lot for doing this! I really like the way it's turning out.
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 also realised that sat config inherits from DropoutMixin (and always has been), so maybe that's the solution? I know we were going to change that to simplify into just delay, so unless there's something I'm missing I think sat and nwp can use the same mechanism here?
Though dropout is also inherited by GSP and Site configs, and I'm not sure if there are use cases for those that will not be covered by delay.
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.
-
Switching to (-60, -15) and delay -30 instead of the current (-90, -45)
Personally I see that as slightly more complicated that it needs to be and I prefer having one less parameter. In the case you suggest (-60, -15) and delay -30 is an identical slice to (-45, 0) and delay -45. I think it is messy that we could get the same time slice with different parameter settings. That feels like something which could be prone to human error too
-
About the use of dropout and delay
I'm not sure if I'm answering your question here, but I think the dropout and delay should be different things. Delay controls the time slice we are choosing and therefore the shape of the input tensor. Dropout controls whether those requested datetimes are actually available or are infilled with NaNs in the array. Admittedly we abuse the "dropout" in the NWP data but I think we should change that to make it clearer.
I think we should get rid of "delay" as we have it currently, and just use the intervals relative to t0. We can then discuss how we name "dropout" across different data sources
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.
Hello again!
- I see what you're saying! I guess I just dislike not having the duration of the slice explicitly stated anywhere. Also, this is beside the point probably, but I think it's fine to have different parameter settings to give you the same slice: if I'm reading someone else's config and it says (-90, -45) I'd assume it's delay 45, slice of 45, and if it's actually delay 30 and we don't want to use the newest 15 min for some reason I'd want to know (don't think this would ever happen but I hope you can see what I'm getting at anyway; sorry if it's not too clear!).
Anyway, honestly, I think this conversation doesn't mean we can't merge this, I think intervaling is a good update regardless and we can always redo delay later if we ever want to.
- Yeah good point! I agree dropout and delay shouldn't be the same mechanism. I think we were going to redo delay for NWP anyway so that should resolve this maybe. One thing though, and that relates to the previous point: it seems strange to me that delay should control the shape of the tensor; I'd expect it to be decided by the time slice exclusively, and delay to just 'slide' it along the time axis, if you will? But maybe there's a reason for it to be like that. Anyway, I feel like I've unnecessarily turned your PR into a debate club, sorry! Gonna go approve it now.
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.
Thanks! We can come back to this again if we want to change it and I'll add it to the agenda of our next data-sampler meeting
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
==========================================
+ Coverage 93.05% 94.66% +1.60%
==========================================
Files 22 27 +5
Lines 691 824 +133
==========================================
+ Hits 643 780 +137
+ Misses 48 44 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
In this PR we:
interval_[start/end]
Closes #22