Skip to content
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

Merge projected and is_geographic #187

Open
veenstrajelmer opened this issue Dec 4, 2023 · 3 comments
Open

Merge projected and is_geographic #187

veenstrajelmer opened this issue Dec 4, 2023 · 3 comments

Comments

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Dec 4, 2023

Missed projected initialization argument when implementing is_geograpic property in #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
Copy link
Collaborator

Huite commented Dec 5, 2023

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:

  • Cartesian
  • Spherical
  • SphericalAccurate

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

@veenstrajelmer
Copy link
Collaborator Author

veenstrajelmer commented Dec 5, 2023

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
Copy link
Collaborator

Huite commented Dec 5, 2023

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:

def test_multiple_coordinates():

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants