iris

iris copied to clipboard

✨ 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:

with iris.experimental.ugrid.PARSE_UGRID_ON_LOAD.context():
    cubes = iris.load(filepath)

So, I think it should also be possible to write :

iris.experimental.ugrid.PARSE_UGRID_ON_LOAD.enable = True
cubes = iris.load(filepath)
cubes_2 = iris.load(filepath2)

or

with iris.experimental.ugrid.PARSE_UGRID_ON_LOAD.context(enable=True):
    cubes = iris.load(filepath)

or even

iris.experimental.ugrid.PARSE_UGRID_ON_LOAD.enable = True
with iris.experimental.ugrid.PARSE_UGRID_ON_LOAD.context(enable=False):
    cubes = iris.load(filepath)

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 private "_state" variable would becomes a public "enable" variable.
• The "context" method api gains an "enable=True" keyword.
• the class already uses "threading.local" to make the control thread-specific. So that already works just the same as FUTURE

@pp-mo ... or even retire the need for this context-manager entirely 🤔

i.e., finally migrate iris.experimental.ugrid into the core of iris.

I'd certainly vote for that 👍

@pp-mo ... or even retire the need for this context-manager entirely 🤔

i.e., finally migrate iris.experimental.ugrid into the core of iris.

I'd certainly vote for that 👍

I was originally a strong advocate for this. But when we first approached it, it turned out there were users who wanted to retain the "old way" that Iris would load UGRID data before the ugrid support.

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 iris as if iris.experimental.ugrid wasn't implemented? As in, load it incorrectly?

In general, I really don't think it's healthy for us to maintain this UGRID context manager pattern/baggage within iris. IMHO it should deprecated and purged.

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 🤔

As in, load it incorrectly?

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.

IMHO it should deprecated and purged.

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. At the same time, we should move it all out of 'experimental', and deprecate accessing it from there, and any use of the context manager. So, at next major release, everything in experimental.ugrid is simply gone, and the content manager no longer exists.

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. I suppose there might be a tiny remaining need, but it's hard to know who we could ask. So maybe we can be bold and just not support "non-ugrid load", from the next (minor) release.

@trexfeathers do you have a view on this ?

it's hard to know who we could ask

except, probably @hdyson Ping !

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 iris.experimental:

So maybe we can be bold and just not support "non-ugrid load", from the next (minor) release.

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?

At the same time, we should move it all out of 'experimental'

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.

@SciTools/peloton ... on by default ... context manager raise a deprecation warning .. don't offer a switch to turn this back off.

Supplemental : to be clear, the plan says the "context manager" will no longer do anything except issue a deprecation warning. If someone objects at the RC stage, we could rewrite to provide a "context = off", but there would have to be a rather good reason.