Closed dfulu closed 10 months ago
@jacobbieker and @peterdudfield, thoughts?
I would be inclined to our own encode_label() option, as we use more PV systems, it could keep the code a bit easier than multiple ID systems. Keeping them consistent might be a bit of work, but probably good long term too
Thanks for putting this all together, its really nice having it all laid out. Ive put some thought into here - https://github.com/openclimatefix/nowcasting_dataset/issues/691, but never finished it.
Few comments
load_everything_into_ram
renames something into what it isnt. That probably made some of the confusion. pv_system_row_number
which I guess is like our encoding. If we want to encode the PV system into the NN model, we probably should use as low numbers as possible. If so, then a CSV with the columns, id, system_id, client_id, provider would probably be needed. You could just use the id's in the database alreadyThe problem with 1, is we will soon have a few systems that are not coming through Sheffield Solar, so that might muck this up.
changing the database is annoying.
is what I've linked too, and perhaps that is the good way to do it. Enocoding like this would work, i'd been tempted to do n -> int(f"{n}{xx}")
where xx i the number of provider. We might quickly go over 10
so 2 digits gives us a bit more room
if you have time not add tech debt, then we should
I agree that load_everything_into_ram()
should be modified.
pv_system_row_number
is computed based on the input dataset and I don't know if it would be a reliable ID to use to identify the system between datasets. Like between training and production. When loading training data, that number comes from the order of the PV systems in the training dataset, like the PV/Passive/ocf_formatted/v0/passive.netcdf
file on GCP rather than being from the metadata csv. The order of the the PV systems in the passive.netcdf
(and therefore the pv_system_row_number
) seems to be quite arbitrary.
Also, unless I'm mistaken, for loading the PV data from the database we set the pv_system_row_number
to 1 for all systems.
Something like 3 might be the best bet
I'm going to close this now. This was fixed by #225 and #226 and by adding ml_id to the training data in order to match the database data
Describe the bug
When using the Passiv PV for training and loading it using
ocf_datapipes.load.pv.pv.load_everything_into_ram()
, the function takes thess_id
ID and renames it aspv_system_id
in the xarray.Dataset returned. In the input metadata, there is also a column namedsystem_id
, which is not carried through to the xarray.Dataset. Both of thess_id
andsystem_id
initially come from the sheffield solar API and they are different.Currently, only the
system_id
ID is available in our database, although it is under thepv_system_id
column there. Also, when we load from the database we encode this ID liken -> int(f"{n}1")
.All this makes it quite difficult to match between the same systems in training and production. Being able to match will be important for adding Passiv inputs to PVNet as per openclimatefix/PVNet#68
Some potential fixes
ss_id
ss_id
available in the databasess_id
is unique for each system across Enphase, Passiv, and PVOutput. This means that we would need to change the library if we wanted to use PV systems outside of these sources.ss_id
(sheffield solar ID?) already hard coded intoload_everything_into_ram()
, so we would likely need to change that anyway.system_id
.get_pv_power_from_database()
and maybe think about the encoding.system_id
throughload_everything_into_ram()
, and that may involve some renaming of the IDs variables.encode_label()
and apply this everywhere consistently.system_id
to the dataset created inload_everything_into_ram()
and select systems based on this.get_pv_power_from_database()
but just select the IDs likesystem_id -> int(f"{system_id}1")