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

Live: Inference - Make GSP not necessary needed #15

Closed
peterdudfield opened this issue Aug 13, 2024 · 12 comments
Closed

Live: Inference - Make GSP not necessary needed #15

peterdudfield opened this issue Aug 13, 2024 · 12 comments
Assignees

Comments

@peterdudfield
Copy link
Contributor

Detailed Description

Currently GSP dataset is needed, but for live inference we infact dont need this.
We will need to use a certain t0 in live

Context

  • prepare, this to be used for live

Possible Implementation

  • TODO
@peterdudfield peterdudfield changed the title Make GSP not necessary needed Live: Inference - Make GSP not necessary needed Aug 13, 2024
@peterdudfield peterdudfield self-assigned this Aug 13, 2024
@peterdudfield
Copy link
Contributor Author

Im unsure where to just make GSP optional, all make a dummy zarr file with fake gsp results.

The dummy file feels like we shouldnt need it, but then adding sun position gets hard without gsp.

@dfulu what do you reckon? Maybe the dummy zarr might be easier

@dfulu
Copy link
Member

dfulu commented Aug 14, 2024

I think we should refactor add_sun_position to not require the GSP data. That is planned in #20

@dfulu
Copy link
Member

dfulu commented Aug 14, 2024

I think this should be easier to do once #27 is merged

@peterdudfield
Copy link
Contributor Author

Yea thanks, yea it should be easier. Unfortunately it still needs the GSP config, which i was trying to get rid of in live. Mokcing the GSP zarr would still be an option.

Other option is to have a training=true, but I like the idea of not using this as it runs the risk of the training and inference pipeline being very differnet

@peterdudfield
Copy link
Contributor Author

One other option could be to have a new sun configration, that we use for making sun coordiantes. We could have some deafult values here that would be basically the same forecast minutes as gsp

@dfulu
Copy link
Member

dfulu commented Aug 15, 2024

Yeh probably having a separate entry in the config would be best. Maybe we should go ahead and bring the datapipes config in here now?

@peterdudfield
Copy link
Contributor Author

Yeh probably having a separate entry in the config would be best. Maybe we should go ahead and bring the datapipes config in here now?

yea why not, can you copy over tests e.t.c too?

Again, it would be great if it was backwards compatiable. I.e if we add sun config, lets add default values to it, so even if an old config doesnt have sun, there will be values

@dfulu
Copy link
Member

dfulu commented Aug 15, 2024

The more issues that come up, the more I think we shouldn't aim for backwards compatibility. In the old version we add the sun position to match the input modality. i.e. to match the datetimes for GSP, or PV, or even wind. So which of these modalities would we make the config default to?

As far as I'm aware Windnet does use the solar coords with the wind modality, but PVNet UK regional uses the GSP modality, and I think PVNet Rajasthan uses the PV modality

@dfulu
Copy link
Member

dfulu commented Aug 15, 2024

And also it would be good to be able to change the config to match the parameter naming of #22.

@AUdaltsova
Copy link
Contributor

AUdaltsova commented Aug 15, 2024

As far as I'm aware Windnet does use the solar coords with the wind modality

It doesn't anymore, we ended up stripping that out when I was adding explicit time features, it uses this instead.

It is very tempting to me to not aim for backward compatibility, yes. I think otherwise we will end up keeping things we don't like but are "fine"/adding props that will need to be stripped out once the migration is complete but will be inevitably forgotten/something else will depend on them by then

@AUdaltsova
Copy link
Contributor

AUdaltsova commented Aug 15, 2024

But also re: having sun position default to something/completely divorced from GSPs, there're already default settings in configs (for history and forecast duration, might even be for time resolution as well but I doubt it; but in any case, surely we can settle on a default time res)? Why can't we use those?

Or maybe even get min/max of intervals and time resolutions of sources? This way we can get exact coverage for the time window we're using at the highest granularity present. I don't know, adding separate configs for it seems odd to me; it kind of makes it its own data source, when in reality it's just a function of preexisting coordinates

@peterdudfield
Copy link
Contributor Author

Could we create an issue called 'moving configuration from ocf_datapipes` and then chat about it there?

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

No branches or pull requests

3 participants