parcels icon indicating copy to clipboard operation
parcels copied to clipboard

Support time varying depth dimensions for xarray datasets

Open apatlpo opened this issue 3 years ago • 6 comments

Follows from issue #1128

Test notebook is on the following gist

apatlpo avatar Jan 20 '22 09:01 apatlpo

I just pushed a small fix to the PR; it turned out that the depth fields were never connected to the grid because the Field.dataFiles is None for Fields created from xarray and therefore the following lines evaluate True https://github.com/OceanParcels/parcels/blob/f6e2ed7f7a2e45eed9acbb2603caf3c124a3c831/parcels/fieldset.py#L271-L272

This seems to be fixed now by changing the last bit to

or (f.dataFiles is None and f.creation_log != 'from_xarray_dataset'):

Can you check if this now works as expected?

erikvansebille avatar Jan 20 '22 17:01 erikvansebille

I think it does indeed, see the gist ! Thanks for your help.

What next, should we try to adresse the large number of tests that are failing? or work on a tutorial?

I also have a somewhat indirectly related question: could you please confirm using xarray does leverage its lazy capability (i.e. does not load full datasets in memory) ?

apatlpo avatar Jan 21 '22 05:01 apatlpo

Great to hear that this fix seems to work, @apatlpo! I've just pushed a set of fixes (one related to PEP8, the other to a bug in reading the depth dimension from the dictionary when depth is set) so hopefully the CI should now give green ticks

I don't think we need a tutorial for this, but I've just added a sentence that this works in FieldSet.from_xarray_dataset() in the timevarying depth dimension tutorial. We do need a unit test though, I'll create that based on your gist and put it in tests/test_grid.py

erikvansebille avatar Jan 21 '22 07:01 erikvansebille

I also have a somewhat indirectly related question: could you please confirm using xarray does leverage its lazy capability (i.e. does not load full datasets in memory) ?

Hmm, good question. To be honest I don't know. We haven't used FieldSet.from_xarray_dataset() in real-life applications yet, so had no need to test/check this. I'd be very keen to hear the answer though; hopefully the lazy capabilities work out of the box!

erikvansebille avatar Jan 21 '22 07:01 erikvansebille

Ok, I'll give a shot to test the lazyness of the code and report back

apatlpo avatar Jan 21 '22 08:01 apatlpo

Ok, I'll give a shot to test the lazyness of the code and report back

apatlpo avatar Jan 21 '22 08:01 apatlpo

Closing this PR for now, as there was no action for a long time

erikvansebille avatar Oct 09 '23 17:10 erikvansebille