PVNet concurrent datapipe #320
Conversation
dfulu
requested review from
jacobbieker and
peterdudfield
and removed request for
jacobbieker
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…imatefix/ocf_datapipes into pvnet_concurrent_datapipe
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 75.65% 75.24% -0.42%
==========================================
Files 126 128 +2
Lines 5994 6208 +214
==========================================
+ Hits 4535 4671 +136
- Misses 1459 1537 +78 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
…imatefix/ocf_datapipes into pvnet_concurrent_datapipe
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…imatefix/ocf_datapipes into pvnet_concurrent_datapipe
for more information, see https://pre-commit.ci
| self.shuffle = shuffle | ||
|
|
||
| def _yield_all_iter(self, xr_dataset): | ||
| # Get the spatial coords |
There was a problem hiding this comment.
could you say what this iter does?
| @@ -0,0 +1,510 @@ | |||
| """Create the training/validation datapipe for training the PVNet Model""" | |||
There was a problem hiding this comment.
should this say something about all gsps?
There was a problem hiding this comment.
I tmight be worth a top summary of this differnet function in here too. Just bullet points of the function name and what they do would be great
| return datapipe | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
remove this, i think its already in a test?
| if from_coords == "lon_lat": | ||
| x, y = lon_lat_to_osgb(x, y) | ||
|
|
||
| # else the from_coords=="osgb" and we don't need to convert |
There was a problem hiding this comment.
if there worth putting and error message, if we dont have the message. or a logger statement if no converstaion is needed
| locidx = xr_data[location_idx_name].values | ||
|
|
||
| # Create a KDTree | ||
| tree = KDTree(list(zip(lat, lon))) |
There was a problem hiding this comment.
I'm not sure. This is old code that I just refactored into functions rather than only have it inside a datapipe. I was refactoring the functions I use so I decided just to do the rest
peterdudfield
left a comment
peterdudfield
left a comment
There was a problem hiding this comment.
Ive left a few comments, but everything else looks good
How much is it worth testing this TODO's before merging? |
I've already got it working locally in the concurrent batch creation script in PVNet, so I'm confident enough to merge |
for more information, see https://pre-commit.ci
This new datapipe is for generating batches, where in each batch there are 317 samples for all of the 317 regional GSPs at the same init time t0. This is much faster (at least 10x faster) than our current method of creating similar batches.
This will allow us to create batches for the summation model and to run backtests much faster.
INCLUDED
TODO:
TODO in PVNet library after merging