use zero for land-values throughout Parcels

[erikvansebille]

Description

This PR implements the definition that land values are zero in Parcels. This choice is intuitive for velocities (since ocean velocities are zero on land) and is needed for slip boundary conditions. For tracer fields, using zero for land also works nicer because NaNs currently throw errors. The XLinearInvdistLandTracer interpolator also assumes that land is 0.0

This PR adds a _assert_no_nan_in_field_data() function that throws a ValueError when there are NaNs in the Sgrid-dataset, and adds fillna(0) calls to some of the convert functions.

This PR supersedes #2451

Checklist

• Closes Land as np.nan or 0 in v4? #2393
• Tests added
• This PR targets the correct branch (main for normal development, v3-support for v3 support)

AI Disclosure

None

[erikvansebille]

Since isnan is not a lazy function, we need to be careful not to check the entire DataArray. Here only checking the first time step, under the assumption that if there is no land in that first time step it will also not be there in other time steps

Admittedly, this is a bit of a hacky fix, since we have to compute the entire first time slice of each Field

If you know of a cleaner way to do this, I'd be very open to that

[VeckoTheGecko]

ah, didn't see this haha. see my other comment for suggestions

[VeckoTheGecko]

This would be a computationally expensive check. Basically it would mean that we look at every value in every field for the first timeslice.

Since assert_valid_field_data is called in assert_valid_model_data, which in turn is called in StructuredModelData.__init__ means this check happens at FieldSet instantiation. Having slow class instantiation can also result in bad user experience, and I think with Copernicusmarine this would be the case.

Do you think you can quickly test the perf of this with copernicusmarine @erikvansebille ?

The options I see are:

1. doing this eager check

• cons: as above

2. doing a ds.fillna(0) internally on the models

• cons: its a potentially redundant operation

3. using dataset metadata (if available)/trusting users to have the right data

• cons: trusting user data

Perhaps (2) just makes the most sense since at least that's done lazily at runtime?

[VeckoTheGecko]

The nice thing about (2) is that we can ensure correctness, and for the performance hungry users we can offer the escape hatch of FieldSet.from_...(skip_field_data_validation=True) giving us (3)

[erikvansebille]

I agree that your option (2) above is the best - and it seems to work well for Dask-arrays. But not for our Zarr backend.. Because if I make the change below

diff --git a/src/parcels/_core/model.py b/src/parcels/_core/model.py
index af208b3f..8c0e21d1 100644
--- a/src/parcels/_core/model.py
+++ b/src/parcels/_core/model.py
@@ -133,11 +133,13 @@ def preprocess_sgrid_model_data(ds: xr.Dataset) -> xr.Dataset:
 
 
 class StructuredModelData(ModelData):
-    def __init__(self, data: xr.Dataset, mesh: Mesh, vector_field_components: ptyping.VectorFields):
+    def __init__(self, data: xr.Dataset, mesh: Mesh, vector_field_components: ptyping.VectorFields, fill_nan_to_zero: bool = True):
         if not isinstance(data, xr.Dataset):
             raise ValueError(f"Expected `data` to be an xarray.Dataset . Got {type(data)}")
 
         data = preprocess_sgrid_model_data(data)
+        if fill_nan_to_zero:
+            data = data.fillna(0)
         grid = XGrid(data, mesh)

And then run pytest tests/test_fieldset.py::test_fieldset_describe_backends
I get

>       assert actual == expected
E       assert "| Name   | T...00000000'))\n" == "| Name   | T...00000000'))\n"
E         
E         Skipping 231 identical leading characters in diff, use -v to show
E         Skipping 309 identical trailing characters in diff, use -v to show
E         -         | Zarr              |
E         ?           ^^^^^
E         +         | NumPy             |
E         ?           ^^^^^...
E         
E         ...Full output truncated (8 lines hidden), use '-vv' to show

So the Zarr array seems to get converted to Numpy. Is that a problem?

[VeckoTheGecko]

I think we should just have one option and call it either fill_nan_to_zero or skip_field_data_validation. The emphasis here is to bypass expensive checks on the data itself.

Other validation (of coordinates, dimension ordering, dimension naming) is cheap and should always be done - hence I don't think we should have assert_valid_fields as an option here.

[erikvansebille]

Ah sorry, then I mistook your comment. I thought you also proposed a way to work around the validation checks in general.

In that case, I prefer skip_field_data_validation, and highlight in the docstring that means no NaN filling. I'll change

[erikvansebille]

Implemented in 2bb3d10