Detangle generation data

[dfulu]

Pull Request

Description

Currently when we run models in production we HAVE to provide generation data, even if the model does not use generation data as an input. We've tangled too much additional responsibility into the generation data like also providing the coordinates and t0-spacing.

This PR detangles the generation data inputs from the rest of the code.

The main changes are:

• Change the generation data schema. We no longer expect longitudes and latitudes in the generation data.
• The location ID to coordinate mapping now lives in a separate metadata file which we need to point to from the config
• We split the generation data into "input" and "target" components. At inference time we can null out the target part of the config, and if a model does not use generation "input" then we don't need generation data at all.
• Various updates to processing and slicing to accommodate the 2 pieces of generation data which might be used.
• We used to tie the datetime encodings to the time interval used for the generation data. This was messy and now that we're splitting the generation in two pieces and not using it at inference, this had to be untied. So I've added a new item to configure the datetimes passed into the datetime encoding function. This is similar to what we do for the solar coords. Incidentally completes Create config input for date/time features #256
• We used to tie the t0-spacing to the generation data intervals in the config. This was messy, and we can't do this if we plan to have no generation. I've moved the value used to define the t0 spacing (and also the path to the locations metadata) to a new config entry sampling_grid.
• The apply_history_dropout() was designed specifically for the old generation data slice so that the future generation wasn't dropped out but the last data was. We no longer need this since we have split the generation data already. I have modified it into apply_dropout() so that all values after the randomly sampled dropout time are filled with NaNs. No dropout is allowed for the target generation data.
• Previously we were using the capacity at the start of the sliced generation data interval to normalise the data. This was just a simplification. I've changed this to use the capacities at each point in the interval

Other changes:

• Unnest the configuration. The top level of the configuration used to have general and input_data sections. We have never used the general section. So I've moved this top level and now the top level is everything which was previously under input_data. I also renamed the config class to PVNetDataConfig to allow us naming room for any future datasets. Completes move input data up a level in config #60
• To make things cleaner for the main changes above I did some refactoring in a few places.

Checklist:

• My code follows OCF's coding style guidelines
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked my code and corrected any misspellings

[dfulu]

This class is used for both the input and target generation slices. The full config paths are config.generation.[input/target]

This class doesn't have time_resolution_minutes or zarr_path since those are assumed to be shared and they live directly under config.generation

[peterdudfield]

Looks great ive left a few comments but nothing big

Do you want to add a migrations script in this PR too? Or perhaps that will come along a bit later

[dfulu]

Looks great ive left a few comments but nothing big

Do you want to add a migrations script in this PR too? Or perhaps that will come along a bit later

Typically we put migration scripts in PVNet when we upgrade it to newer versions of ocf-data-sampler. I think we should do the same. Plus we should keep an eye on how many breaking changes we're introducing. I think we should consider a clean break

[AUdaltsova]

I am a bit in two minds about this test; I think it duplicates a lot of things done by tests in tests/select/test_dropout.py, as all this function does is check for keys and call apply_dropout, which is already extensively tested in the dropout section. I guess it can stay to catch if someone accidentally makes the if statements nested or something... But I think it can maybe use _generation_da() instead of re-making generation DataArray?

[AUdaltsova]

Okay actually, I've noticed it's not even testing satellite dropout, so it will not tell you anything useful that apply_dropout wouldn't I think?

[dfulu]

Yeh I see what you mean, and I am a fan of less code.

This test does catch if everything in the configs and datasets are unpacked and repacked correctly. Like did we take the right settings from the config and apply the correct dropout to each data source. The generation settings are under config.generation.input but the satellite setting are shallower under config.satellite. But the tests in test_dataset.py should catch that too, so that seems like a strike against it. This additional test would only guard against us going something silly like using the satellite dropout settings into the generation.

The apply_dropout_to_datasets() function is advertised as an in-place operation, and this test maybe does help lock that in. It could help us catch errors if we ever made (accidentally or otherwise) apply_dropout_to_datasets() out-of-place.

Note: I do plan to come back and make us more consistent with what we return for in-place and out-of-place functions. Sometimes we return an object for an in-place operation and sometimes we don't.

I don't feel strongly either way. What do you think?

[AUdaltsova]

Okay yeah I think it does make sense to check that we've passed the right config along. I think in that case it should check that dropout is applied to satellite (currently satellite has settings that don't result in dropout; I think the result of that is ambiguous - if nothing happened it can mean apply_dropout() wasn't called at all), and also maybe check that nothing happens to generation_target? That feels like something a silly update might end up breaking.

I think the fact that it's in-place is locked in apply_dropout() as that's already in-place? You can't really change that by only changing apply_dropout_to_datasets().

I also still think the _generation_da() from this file can probably be reused here instead of LL 105-113, but maybe I'm missing something?

Also, I do realise I am nitpicking over a test that was already kinda bad before this PR. If you think this is getting silly, go ahead and merge as is, LGTM otherwise!

[dfulu]

Yes it is locked in, because of apply_dropout(), but there is always the chance we might decide to make apply_dropout() out-of-place later and forget to update this function. If we did that we'd be silently not apply dropout anywhere and I think this is the only test that might catch that. But that is an real edge case, we shouldn't be so silly.

So how about this this then. We switch to use _generation_da() as a helper, we add generation_target to the sample and check it isn't touched, we add dropout to the satellite and we merge this? I'm going to do a sweep through the tests in a later PR anyay