-
Notifications
You must be signed in to change notification settings - Fork 283
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
read-write Xarray with nc-dataset adapter. #4835
Conversation
Status update: |
raise IOError(msg) | ||
else: | ||
raise | ||
if hasattr(filename, "createVariable"): |
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.
Is it worth considering a runtime protocol to define, in code, what you mean by "supporting the netCF4.Dataset api"?
https://docs.python.org/3/library/typing.html#typing.runtime_checkable. Added in Python 3.8.
@@ -1061,6 +1064,7 @@ def __init__(self, filename, netcdf_format): | |||
|
|||
* filename (string): | |||
Name of the netCDF file to save the cube. | |||
OR a writeable object supporting the netCF4.Dataset api. |
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.
OR a writeable object supporting the netCF4.Dataset api. | |
OR a writeable object supporting the netCDF4.Dataset api. |
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.
Not a complete review, just some questions that popped out at me as I read the code.
This is SUPER cool :D
class _XrMimic: | ||
""" | ||
An netcdf object "mimic" wrapped around an xarray object, which will be | ||
either a dim, var or dataset. |
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.
By dim do you mean DimCoord, in iris parlance?
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.
might be good to be explicit here: var I assume is xr.Variable
, dataset is xr.Dataset
. dim is an object that doesn't exist in xarray, but you need it for netcdf compat?
|
||
def __init__(self, xr, name=None, group=None): | ||
""" | ||
Create a mimic object wrapping a :class:`nco.Ncobj` component. |
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.
What is nco.Ncobj
? Old init string text?
|
||
def __getitem__(self, keys): | ||
if self.ndim == 0: | ||
return self._xr.data |
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.
keys
is ununsed in this branch of the if. Is that right? Should keys
have default value? Document with a comment either way I think.
Only a limited subset of the :mod:`netCDF4` APIs are currently | ||
supported : just enough to allow Iris to read and write xarray datasets | ||
in place of netcdf files. |
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.
Again, this might be where a runtime protocol could help document which parts of the api are implemented so far.
Lazy xarray outputHere's an interesting follow-on possibility ... pp-mo#73 without this, iris "streams" all data into the netcdf variables, as it would to a file, |
Important Context : things this can be used for ?In addition to just "fixing data a bit" on either load or save,
Generally, all these issues may be better handled in Xarray, since they are close to the file representation -- so a solution there can work with specific variables+dimensions, rather than Iris CF-representing objects. |
Ping @dcherian Just thought that I'd make you aware of this PR. It's early days, but you may be able to offer some perspective on this... |
Thanks for making the link @bjlittle FWIW some other comments on this that you might also like to consider .. The current approach here is rather untidy, and grew out of an attempt to create a simple 'wrapper' object to an existing xarray dataset. But that basic approach actually fails once write support is added, since it is not possible to modify the dimensions of an existing xarray dataset, and the result is now a rather clunky "mixed" approach. The principal limitation of this as it is, is that it expects an xarray-dataset which does not intpret cf/coords/time-data
] If this "load/save xarray as a netcdf dataset" approach has some general utility, then we should put it into xarray (not Iris!), |
Thanks @bjlittle Is there an issue I can read about the motivation for this? If it is to take advantage of Xarray's read/write backend options, would it be easier to use the |
You're quite right, there should probably be an issue.
It seems to me that a big "problem" with xarray Meanwhile, there is a reasonable argument that an And with both those solutions, there is a particular problem regarding dependencies and testing, especially for C.I. :
However, this kind of "dataset adapter" approach only depends on knowledge of xarray and netCDF4.
(**) but we still need certain changes in Iris :
Hence, code here is really just a PoC, showing how those solutions can work together |
I have now broken this by advancing the F-B with a mergeback. So the "let iris read+write nc datasets" part is now up, in #5024, |
Targets feature branch, just to unify testing.
This introduces a "netcdf dataset adapter" for xarray datasets, enabling to read/write xarray data as if it were a netcdf file.
ALSO combined here (and not really separate for now):
iris : _generate_cubes
iris.io : decode_uri
iris.io.format_picker
If this idea flies, it might be worth separating out the core-iris changes into a separate PR rather than doing it all at once?
Hence "WIP"
Also, various TO-DOs noted on this old "private PR" : pp-mo#72(update: all done)