fix: support conditional UKV renaming with tests #364#393
fix: support conditional UKV renaming with tests #364#393dfulu merged 6 commits intoopenclimatefix:mainfrom
Conversation
tests/load/nwp/providers/test_ukv.py
Outdated
| import xarray as xr | ||
|
|
||
|
|
||
| def test_ukv_rename_logic_legacy(): |
There was a problem hiding this comment.
@ArkVex these tests are not best practice. Its standard to test the actual function rather than the logic internal to the function and these tests would be a pain to maintain. The legacy version of this function will already be run in this test. You could create another one which lives alongside that test which tests the newer coords
|
|
||
| # Only rename if the source key exists in the dataset's dimensions or coordinates | ||
| # This prevents KeyErrors when the new UKV data already has "x_osgb" and "y_osgb" | ||
| actual_rename = {k: v for k, v in rename_map.items() if k in ds.dims or k in ds.coords} |
There was a problem hiding this comment.
There is some redundancy here. It would be simpler and good enough just to check if k in ds.coords
|
@dfulu plz check the applied changes |
| # This prevents KeyErrors when the new UKV data already has "x_osgb" and "y_osgb" | ||
| actual_rename = {k: v for k, v in rename_map.items() if k in ds.coords} | ||
| if actual_rename: | ||
| ds = ds.rename(actual_rename) |
There was a problem hiding this comment.
@ArkVex just one last comment. These lines could just be reduced to
ds = ds.rename({k: v for k, v in rename_map.items() if k in ds.coords})
which will work even if an empty dictionary is input
There was a problem hiding this comment.
Here you go...I have implemented what you asked
|
@dfulu thankyou so much |
|
@dfulu one thing i wanted to ask |
|
@ArkVex I don't know yet. Internally we're still only at the brainstorming stage for projects. So it will still take some time. If you haven't seen it already our community discussions page is the first place we'll announce the projects |
2 participants
Pull Request
Description
Fixes #364
This PR implements conditional renaming for UKV data coordinates in the
open_ukvloader. Previously, the code strictly expected legacy dimension names (x,y), which causedKeyErrorexceptions when attempting to load newer UKV datasets that already use thex_osgbandy_osgbnaming convention.Changes:
.rename()call with a dictionary comprehension that checks for the existence of dimensions/coordinates before attempting to rename them.How Has This Been Tested?
I have verified the fix by creating a new test file
tests/load/nwp/providers/test_ukv.pywhich includes:x,y,init_time, andvariableare correctly renamed.Tests were verified locally using
pytest.If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?
Checklist:
CC @dfulu plz review