-
Notifications
You must be signed in to change notification settings - Fork 138
Fix longitude coord in cmorizers utilities.py #3911
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
base: main
Are you sure you want to change the base?
Conversation
@@ -221,7 +221,7 @@ def fix_coords(cube, | |||
if cube_coord.ndim == 1: | |||
cube = cube.intersection(longitude=(0.0, 360.0)) | |||
if overwrite_lon_bounds or not cube_coord.has_bounds(): | |||
fix_bounds(cube, cube_coord) | |||
fix_bounds(cube, cube.coord('longitude')) |
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 tackling this @flicj191 - we have to be careful here what of the two conditionals is stopping the call to fix_bounds: either overwrite_lat_bounds
is False or cube.coord('latitude').has_bounds()
is True - in which case no fixing is needed, though overwrite_lat_bounds
is a bit of a loose constraint, maybe they do need fixing, and that was set to False by mistake?
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 V!
hmm, no, overwrite_lat_bounds
would be True, I was calling fix_coords()
with the defaults. the output didn't have longitude bounds and the initial cube didn't have bounds either. The fix_coords()
call when I used it to help CMORize fixed the depth, time and latitude coordinates but not the longitude.
I'll do a few more checks but I think it just got lost when getting parsed through fix_bounds()
ie. latitude is done like line 229:
if overwrite_lat_bounds or not cube.coord('latitude').has_bounds():
fix_bounds(cube, cube.coord('latitude'))
whereas longitude here :
if overwrite_lon_bounds or not cube_coord.has_bounds():
fix_bounds(cube, cube_coord)
I'll have another look and see
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.
just be sure that cube_coord
is indeed the "longitude" one - you never know with those raw datasets, maybe it's called "ducks" 🦆 (am not even joking, the amount of weird stuff I've seen in raw outputs, ducks may be even CMOR-like 😁 )
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.
yep so i'm proposing to replace cube_coord
with cube.coord('longitude')
I haven't had a chance to test it all, will soon
I am a little bit confused as to why this fix is necessary. Shouldn't I also noticed that the metadata that is set during |
Thanks, yes they should be the same, I was just finding that |
Description
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
New or updated data reformatting script
Documentation is availableThe dataset has been added to the CMOR check recipeThe dataset has been added to the shared data pools of DKRZ and Jasmin by the @ESMValGroup/OBS-maintainers teamNumbers and units of the data look physically meaningfulTo help with the number of pull requests: