Deltares / xugrid

Xarray and unstructured grids
https://deltares.github.io/xugrid/
MIT License
58 stars 8 forks source link

Merge `projected` and `is_geographic` #187

Open veenstrajelmer opened 8 months ago

veenstrajelmer commented 8 months ago

Missed projected initialization argument when implementing is_geograpic property in https://github.com/Deltares/xugrid/pull/186

Would be cleaner if these two can be aligned.

However, the projected property is not properly set:

import xugrid as xu

uds = xu.data.adh_san_diego()
uds_grid = uds.grid
print(uds_grid.projected) # is False but should be True, since crs is not parsed

uds = xu.data.adh_san_diego()
uds_grid = uds.grid
uds_grid.set_crs(4326)
print(uds_grid.projected) # is False and should be False, wgs84 is not projected

uds = xu.data.adh_san_diego()
uds_grid = uds.grid
uds_grid.set_crs(28992)
print(uds_grid.projected) # is False but should be True, RD is projected

Returns:

False
False
False

Should return:

True
False
True
Huite commented 8 months ago

Probably good to know, the main reason I set the projected property is because I ran into problems with multiple coordinate systems, i.e. a dataset that listed both x and y coordinates as well as longitude and latitude.

It's currently only used to determine which names to write ("latitude" or "y") and the attributes of the coordinate.

Regarding the naming: these are obviously the same. I didn't like is_geographic too much, but it seems to be common in GIS parlance. Meshkernel also has this:

https://deltares.github.io/MeshKernelPy/api/meshkernel.py_structures.html#meshkernel.py_structures.ProjectionType

Which mentions:

I guess I'm okay with either cartesian being either True or False. is_geographic is fine too. projected should probably go.

veenstrajelmer commented 8 months ago

Probably good to know, the main reason I set the projected property is because I ran into problems with multiple coordinate systems, i.e. a dataset that listed both x and y coordinates as well as longitude and latitude.

Do you still have such an example dataset or is it in the testbank? It would be good to check if reading this does not fail with code changes that might come from this issue. I expect this was generated by FM with NcWriteLatLon=1 in the mdu file.

Regarding the naming: these are obviously the same. I didn't like is_geographic too much, but it seems to be common in GIS parlance.

is_projected is also fine in my view (instead of is_geographic), both are also used in pyproj. Or maybe both, that one is just the opposite of the other. But I guess a property instead of a settable would be useful.

Meshkernel also has this: https://deltares.github.io/MeshKernelPy/api/meshkernel.py_structures.html#meshkernel.py_structures.ProjectionType

I am aware of this, but outside of meshkernel the distinction in different spherical/geometric properties has no added value as far as I know.

Huite commented 8 months ago

Do you still have such an example dataset or is it in the testbank? It would be good to check if reading this does not fail with code changes that might come from this issue.

Here's is where it's used:

https://github.com/Deltares/xugrid/blob/main/xugrid/ugrid/conventions.py#L61

There are some tests here: https://github.com/Deltares/xugrid/blob/699452bad2b1cd6bfaae3f6827615aceda60e215/tests/test_ugrid_dataset.py#L762