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

ESPG=4326 is not recognised #69

Open
tomsail opened this issue Nov 21, 2023 · 3 comments
Open

ESPG=4326 is not recognised #69

tomsail opened this issue Nov 21, 2023 · 3 comments

Comments

@tomsail
Copy link
Contributor

tomsail commented Nov 21, 2023

This is one of the reasons that the CI actions don't pass:

Some of the examples in the README use EPSG = 4326 as an integer for the coordinate system.
However in edgefx.py, only strings are tested i.e. :

if crs == "EPSG:4326":

like in bathymetric_gradient_sizing_function() and wavelength_sizing_function()

if crs == "EPSG:4326" or crs == 4326:
seems to be a good fix

@pmav99
Copy link

pmav99 commented Nov 21, 2023

seems to be a good fix

The function docstring says:

    crs: A Python int, dict, or str, optional
        The coordinate reference system

if that is the case, and a dict is acceptable too, then you should probably take that into account, too.

That being said, since crs is an argument that is used in many functions, fixing all the instances is probably out of scope for #68. "Syncing" the code with the docs should probably happen in a dedicated PR. In that case, replacing the integers with strings in the tests is probably more appropriate.

@tomsail
Copy link
Contributor Author

tomsail commented Nov 21, 2023

Actually as I progress doing our Iceland test case, I found more bugs/fixes to implement in various functions.
I agree that correcting & addressing different issues with different PR is the way to go, although it might be confusing for @krober10nd to understand what's going on.

if that is the case, and a dict is acceptable too, then you should probably take that into account, too.

some functions implement the CRS class from pyproj but it is not consistent across the whole package

@krober10nd
Copy link
Collaborator

@tomsail no, I can follow. feel free. thanks for putting in the effort here.

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

3 participants