-
Notifications
You must be signed in to change notification settings - Fork 281
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
Support global enable of ugrid loading #5940
Comments
@pp-mo ... or even retire the need for this context-manager entirely 🤔 i.e., finally migrate I'd certainly vote for that 👍 |
I was originally a strong advocate for this. I don't know how to answer the question, whether those concerns are all gone now. |
@pp-mo Just to clarify, there are users who want to load UGRID using In general, I really don't think it's healthy for us to maintain this UGRID context manager pattern/baggage within If that makes it a breaking change, then we have to discuss that as a realistic option here. |
... at the very least it should be that UGRID loading is enabled by default. i.e., it's an explicit opt-out rather than an opt-in. I'd find that an acceptable compromise as opposed to a hard breaking change 🤔 |
Well no, just "as if" it were plain netcdf-CF data, because none of the extra concepts actually breaks anything in CF (except, technically, their extended use of 'cf_role' 😩 ). But TBH I think this "need" is probably long-since gone. |
I agree, I think we can argue it's not breaking change to just turn this on, by default at least. Because I just don't think we will ever see files that "appear" to be UGRID but are not. One final question for me is whether there is a any role here for a 'legacy' (off) mode at all. Again I think we maybe mis-stepped by providing less interface than the FUTURE flags, so the existing context() api doesn't let you turn it off within a scope. @trexfeathers do you have a view on this ? |
except, probably @hdyson |
I'm happy, we've deprecated and retired the functionality that relied on this 👍 The only thing that will break that I care about is some of the material I use when explaining the UGrid file format - but I can update this to use netCDF4-python or xarray (and I should have probably updated this material by now anyway). |
Just flagging this in case it's an oversight: since when I first read this, I assumed it would only involve changes in
This would be changing the behaviour of iris.load without using the UGrid context manager - should that be a minor release thing rather than a major release? |
Scope creep! |
@SciTools/peloton We like the idea of turning this on by default, having the context manager raise a deprecation warning and, for the sake of code simplicity, we don't offer a switch to turn this back off. Because this code is experimental, this can happen in a minor release and changes to load behaviour can be consideredthe proper introduction of previously experimental code. |
Supplemental : |
✨ Feature Request
The PARSE_UGRID_ON_LOAD control behaves much like the FUTURE flags object, except that there is no public API for turning it on permanently.
So, we can currently say:
So, I think it should also be possible to write :
or
or even
Motivation
This obviously would be more convenient in some user cases.
Plus, it matches the usage interface for FUTURE flags.
Possible implementation + API changes
The text was updated successfully, but these errors were encountered: