search

openclimatefix / PVNet

PVnet main repo
MIT License
21 stars 5 forks source link

fixing current tests bug #243

Closed peterdudfield closed 3 months ago

peterdudfield commented 3 months ago
Screenshot 2024-08-06 at 23 03 07
AUdaltsova commented 3 months ago

Ok so I don't know why this test didn't trip up the previous merge, it should have (when I roll back to the versions at merge locally, it happens as well), but here's what I found out: the problem is that me adding the new keys to ocf_datapipes/ocf_datapipes/batch/batches.py somehow messes with the right batch keys getting assigned to the right tensors from pre-saved test batches. This data was supposed to have this key: satellite_t0_idx, which would've been fine. I bet if I go comment out the new keys in the failing PR it will pass this test

AUdaltsova commented 3 months ago

Yep that 100% on me, I stuck new keys in the middle of an enumerated class. The test is fine

peterdudfield commented 3 months ago

No problem @AUdaltsova, it happens.

Yea we have had this problem before with Enums. Something to think about in the future for ocf_data_samples - https://github.com/openclimatefix/ocf-data-sampler/issues/7

probably a few options

  1. Open up current batches, and load with old enums, and save with new enums.
  2. try moving the new wind enums to the end of the Batch, and then hopefully all the others are in the correct order. I know this breaks the nice grouping of variables

I'd probably be tempted to do 2., but up to you

AUdaltsova commented 3 months ago

@peterdudfield yes I thought the same https://github.com/openclimatefix/ocf_datapipes/pull/351