OpenDrift / opendrift

Open source framework for ocean trajectory modelling
https://opendrift.github.io
GNU General Public License v2.0
247 stars 120 forks source link

adding xarray dataset option to ROMS reader #1214

Closed rsignell closed 8 months ago

rsignell commented 8 months ago

Following the pattern of the generic CF reader, adding the option to pass a xarray dataset to the ROMS native reader.

I wasn't sure if we wanted an example using this approach, because it's just a minor mod of the existing example_roms_native.py test.

(odrift) rsignell@OSC:~/repos/opendrift/examples$ diff example_roms_native.py  example_roms_native_xarray_dataset.py
9a10
> import xarray as xr
14,15c15
< # Creating and adding reader for Nordic 4km current dataset
< nordic_native = reader_ROMS_native.Reader(o.test_data_folder() +
---
> filename = (o.test_data_folder() +
16a17,21
>
> # Creating and adding reader for Nordic 4km current dataset
>
> ds = xr.open_dataset(filename, decode_times=False)
> nordic_native = reader_ROMS_native.Reader(ds)
knutfrode commented 8 months ago

Yes, this looks fine. But I agree that the example might not be needed, as it does not add much more than illustrating that the reader can take Xarray datasets as an alternative to a filename or URL - which should now also be clear from the docstring.

It might be more interesting to show an example illustrating usage of intake - however, this would require four more dependencies (intake, intake-xarray, zarr, s3fs), and we would like to keep dependencies to a bare minimum. And generally it is probably better to avoid direct links between intake and OpenDrift, and instead let Xarray be the common meeting point. But intake does look interesting, we will get familiar with it!

Thus, if you could remove the example from this PR, we can then merge it.

gauteh commented 8 months ago

Those will be optional dependencies, so I think it would be fine to add them. We could either manually add them to CI or add them to environment.yml (or create an extras-environment.yml). Then I think it would be useful to have the intake example.

knutfrode commented 8 months ago

Ok, maybe the simplest first step would be if rsignell could add these minimum dependencies to environment.yml and update the example script to using the intake catalog as in #1188 (assuming the test dataset will remain on the server for some time). Then we could eventually later move these dependencies to an extras-file, if it should be needed.

rsignell commented 8 months ago

I've removed the simple ROMS xarray dataset example in the interest of getting this merged quickly! 😸

With regard to a nice OpenDrift example accessing data using Intake, we have an Intake Catalog that points to public ROMS output hosted by the AWS Open Data program , which would be perfect.

But that dataset uses Parquet references (to improve the cloud performance accessing data from NetCDF files), and running an OpenDrift example on it turned up a bug in fsspec that is fixed in master and should be available in the next release.

So I promise to provide another PR that gives an example using Intake, and an environment that will run it (which will include another optional dependency -- fastparquet).

Sound good?

knutfrode commented 8 months ago

Yes, this is good. Merging now.

rsignell commented 8 months ago

Thanks so much @knutfrode and @gauteh for bringing this one home!

We've had some issues using pip and custom conda channels installing OpenDrift into conda environments using conda-store on Nebari, and being able to install this latest functionality from conda-forge would really help us out!

To facilitate a new version of OpenDrift on conda-forge, would it be too much to ask for a new tag?

knutfrode commented 8 months ago

We will most probably make a new release for Conda within a week or two

gauteh commented 8 months ago

Hi, I have made a new release now.

rsignell commented 8 months ago

@gauteh thanks so much! I see the feedstock action is failing: https://github.com/conda-forge/opendrift-feedstock?tab=readme-ov-file#current-build-status
I'm going to ask @ocefpaf help...

gauteh commented 8 months ago

No, no need to ask. If you see the logs it's just an outdated dependency. You can make a PR with updated deps.