-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Incorporate SiteSample and UKRegionalSample into save/load #311
Incorporate SiteSample and UKRegionalSample into save/load #311
Conversation
for more information, see https://pre-commit.ci
pvnet/data/site_datamodule.py
Outdated
sample = convert_netcdf_to_numpy_sample(ds) | ||
return sample | ||
sample = SiteSample.load(self.sample_paths[idx]) | ||
return sample.to_numpy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment that you could add for extra clarity
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev-data-sampler #311 +/- ##
===================================================
Coverage ? 60.86%
===================================================
Files ? 28
Lines ? 1817
Branches ? 0
===================================================
Hits ? 1106
Misses ? 711
Partials ? 0 ☔ View full report in Codecov by Sentry. |
So I reckon as @dfulu had previously suggested here #301 (comment) for the loading and to_numpy bits this can be abstracted a bit more to essentially one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these changes in, I think just needs a bit more refactoring to get the full benefit of having the generalised interface
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for adding this!
for more information, see https://pre-commit.ci
save_samples.py, site_datamodule.py and uk_regional_datamodule.py updated accordingly to utilise new SiteSample and UKRegionalSample save/load functionality.
Additional test added for clarity and version update stated in dependencies.