Skip to content
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

feat: added spatial coarsening for input #115

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

alirashidAR
Copy link
Contributor

@alirashidAR alirashidAR commented Jan 10, 2025

Pull Request

Description

Issue #92
Added the coarsen_data function in ocf_data_sampler/load/utils.py to coarsen the data to a specified resolution in degrees.

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

@alirashidAR alirashidAR changed the title fix: added spatial coarsening for input feat: added spatial coarsening for input Jan 10, 2025
@alirashidAR
Copy link
Contributor Author

@Sukh-P, I think the file path being used for testing is incorrect.

ERROR tests/torch_datasets/test_pvnet_uk_regional.py::test_pvnet_no_gsp - FileNotFoundError: Unable to find group: file:///home/runner/work/ocf-data-sampler/ocf-data-sampler/tests/test_data/non_hrv_shell.zarr.zip

@peterdudfield
Copy link
Contributor

You ok to merge in the latest main branch? This should get tests passing

@alirashidAR
Copy link
Contributor Author

Yes , thank you !

@alirashidAR alirashidAR reopened this Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.34%. Comparing base (ad6d738) to head (6f04314).
Report is 50 commits behind head on main.

Files with missing lines Patch % Lines
ocf_data_sampler/load/utils.py 22.22% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   94.71%   92.34%   -2.37%     
==========================================
  Files          39       40       +1     
  Lines        1060     1163     +103     
==========================================
+ Hits         1004     1074      +70     
- Misses         56       89      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alirashidAR
Copy link
Contributor Author

@peterdudfield is this ready to merge or any other changes needed?

@Sukh-P
Copy link
Member

Sukh-P commented Jan 23, 2025

Hey @alirashidAR I think this is missing a unit test, there is an example of a test here and you could use this test data which is constructed here, hope that is okay to add, thank you!

Also this function I think would be better suited in this file as this is where it will actually be called and it won't be used anywhere else currently, it won't be called in any of the functions in the load folder

@@ -1,5 +1,6 @@
import xarray as xr
import pandas as pd
import numpy as np
Copy link
Member

@Sukh-P Sukh-P Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this import can be removed now, not blocking merging in but would be nice to clean up before we do! Thanks

Copy link
Member

@Sukh-P Sukh-P left a 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 in!

@alirashidAR
Copy link
Contributor Author

Thank you for the approval !

@Sukh-P Sukh-P merged commit 5bf5d0f into openclimatefix:main Jan 24, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants