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

saving cubes with masked cell measures #5147

Closed
valeriupredoi opened this issue Feb 2, 2023 · 8 comments · Fixed by #5181
Closed

saving cubes with masked cell measures #5147

valeriupredoi opened this issue Feb 2, 2023 · 8 comments · Fixed by #5181
Assignees

Comments

@valeriupredoi
Copy link

Hey guys, how's it going? We're hitting an issue with saving cubes that have masked cell measures attached to the main variable, see ESMValGroup/ESMValCore#1917 - I don't really understand the issue with this, so long as one supplies the saver with a valid fill_value (like we do, the infamous 1e20) - is this something that's a bit legacy or you're still afraid of it? Cheers muchly 🍺

@trexfeathers
Copy link
Contributor

@valeriupredoi this originally stemmed from a desire to align as-close-as-possible to CF. But from discussion today we can't see a practical reason why cell measures shouldn't be masked.

So we'll leave this open as a future development task. If this is a priority for you please let us know.

@pp-mo
Copy link
Member

pp-mo commented Feb 8, 2023

we can't see a practical reason why cell measures shouldn't be masked.

Oh dear, I think it was me said that.
Now I looked at the code, where this is specifically dis-allowed for cell-measures only

This test was added as a specific exclusion in this PR
-- though the code has since been refactored somewhat, so it's not that obvious.

The argument given is "We can't save masked points properly, as we don't maintain a fill_value"
I guess the danger envisaged is that we might have masked data with a specific fill-value set on the array, and we don't support any way of making that the fill-value actually recorded in the netcdf file, so it could generate missing-point collisions.

Altogether It's a pretty theoretical idea of a potential problem : points masked in the array will be saved as default-missing-value, but there might also be unmasked points with that value : Highly improbable, except for integer data.
And this code is supposed to detect and (at least) warn about that occurrence.

So, I really don't agree with this argument -- and especially as it should apply equally to aux-coords, and ancillary variables (added much more recently), and those we don't disallow.
( N.B. the user can avoid the problem for cube data (only), by specifying fill_value(s) in the save operation )

So I think if all agree we could quickly make a change to just remove this check
Any contrary opinions ? speak now !

@valeriupredoi
Copy link
Author

cheers gents! Not a priority nor anything stringent for us, just thought I'd raise a flag or two - if you guys decide to keep it we'd be finding a workaround, and in fact, one of @bouweandela 's current PRs (in ESMValCore) is inadvertently fixing this 👍

@valeriupredoi
Copy link
Author

cheers gents! Not a priority nor anything stringent for us, just thought I'd raise a flag or two - if you guys decide to keep it we'd be finding a workaround, and in fact, one of @bouweandela 's current PRs (in ESMValCore) is inadvertently fixing this +1

Nevermind the second part of my statement - that was a bug, that is now fixed, so we're back at hitting this. It'd be cool if you guys removed the raise as @pp-mo mentions, or both the check and raise 😁

@valeriupredoi
Copy link
Author

hey guys - we're still hitting this with iris=3.4.1 - not sure why you folks closed the issue 😕

@trexfeathers
Copy link
Contributor

hey guys - we're still hitting this with iris=3.4.1 - not sure why you folks closed the issue 😕

Hi @valeriupredoi, this feature was first available in v3.5.0rc0. See 897a7cb:

image

@valeriupredoi
Copy link
Author

ahaa cheers @trexfeathers - let me update then, thanks for the heads up, I jumped the (version) gun

@valeriupredoi
Copy link
Author

with iris=3.5 all goes smoother than a Ferrari's paint work, many thanks @trexfeathers and the iris gang 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants