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

LAEA projection odd handling of NaN #3596

Closed
djhoese opened this issue Jan 26, 2023 · 10 comments · Fixed by #3603
Closed

LAEA projection odd handling of NaN #3596

djhoese opened this issue Jan 26, 2023 · 10 comments · Fixed by #3603
Labels

Comments

@djhoese
Copy link
Contributor

djhoese commented Jan 26, 2023

Example of problem

$ echo NaN NaN | cs2cs EPSG:4326 EPSG:3572
4504982.38      7802858.37 0.00aN NaN

Pyproj example of the in-code issue:

from pyproj import Transformer
import numpy as np

# LAEA target CRS
t = Transformer.from_crs('EPSG:4326', 'EPSG:3572')
t.transform(np.nan, np.nan)
# (0.0, 0.0)

# Mercator target CRS
t = Transformer.from_crs('EPSG:4326', 'EPSG:3857')
t.transform(np.nan, np.nan)
# (nan, nan)

Problem description

Short answer: LAEA projections convert NaN degrees to 0. Other projections (ex. mercator, LCC, etc) return NaN.

I've filed a bug with pyproj which is my primary way of using the PROJ library: pyproj4/pyproj#1239

I'm not familiar enough with the PROJ C API to quickly make an example in C so I tried using cs2cs, but as you can see above something weird happens with the formatting even though my understanding is that strtod should accept "NaN" to represent NaN.

Expected Output

I expect projection transformations of NaN to return NaN on either side of the transformation (forward or inverse).

Environment Information

  • PROJ version (proj): Rel. 9.0.0, March 1st, 2022
  • Operation System Information: PopOS/Ubuntu

Installation method

  • conda via conda-forge
proj                      9.0.0                h93bde94_1    conda-forge
pyproj                    3.3.1           py310h9bf108f_0    conda-forge
@djhoese djhoese added the bug label Jan 26, 2023
@djhoese
Copy link
Contributor Author

djhoese commented Jan 26, 2023

Also, I'm pretty sure this line is the "problem":

} else
xy.x = xy.y = 0.;
break;

And I noticed that NaN from x/y space (inverse transformation) produces NaN in degrees.

@rouault
Copy link
Member

rouault commented Jan 26, 2023

I would say that handling of NaN or more generally non-finite input x,y coordinates is unspecified behavior currently in PROJ, and I'm not sure we need to specify it. This should be rather dealt on your side.

@djhoese
Copy link
Contributor Author

djhoese commented Jan 26, 2023

Thanks for the quick response. I can understand that. I guess my only argument against that is consistency between the different projections and even the forward and inverse transformation of a single projection. The behavior being undefined is still an answer to this, but unfortunate for performance on my end.

Related: #2376

I suppose a PR "fixing" the laea projection would not be accepted?

@djhoese
Copy link
Contributor Author

djhoese commented Jan 26, 2023

I also noticed @kbevers's commit (kbevers@b606396) where mercator was rewritten but still "NaNs are properly handled".

@rouault
Copy link
Member

rouault commented Jan 26, 2023

I suppose a PR "fixing" the laea projection would not be accepted?

I'm not opposed to this being fixed, but this should be fixed somewhere higher in the chain than in each individual projection, conversion or transformation method. Probably in proj_trans() itself.

@djhoese
Copy link
Contributor Author

djhoese commented Jan 26, 2023

Yeah, I suppose we've just been depending on the idea that any math on NaN should be producing a NaN. This is one of the few cases where the conditions just worked out toward a non-NaN result.

@djhoese
Copy link
Contributor Author

djhoese commented Feb 2, 2023

I'm finally starting to look into this. I assume you all would like a test. Could you point me to a good spot to add the test(s)? Somewhere that has similar checks perhaps?

Also, I ran the tests locally and I get a couple failures, is this expected?

90% tests passed, 6 tests failed out of 61

Total Test time (real) = 161.83 sec

The following tests FAILED:
         45 - testvarious (Failed)
         48 - testntv2 (Failed)
         49 - testprojinfo (Failed)
         55 - proj_test_cpp_api (Failed)
         56 - gie_self_tests (Failed)
         57 - test_network (Failed)
Errors while running CTest

@rouault
Copy link
Member

rouault commented Feb 2, 2023

I assume you all would like a test

test/gie/builtins.gie could be a potential host, but I'm not sure it accepts NaN coordinates currently. That might require extending src/apps/gie.cpp. Otherwise in test/unit/gie_self_tests.cpp

Also, I ran the tests locally and I get a couple failures, is this expected?

if run on master without any change, no that's not expected. Tests run fine on all our CI platforms. Maybe check that PROJ_NETWORK=ON is not set in your environment, as I suspect it could cause issues (just trying in my dev env, I see the same tests that fail for you fail for me when I define PROJ_NETWORK=ON)

@djhoese
Copy link
Contributor Author

djhoese commented Feb 2, 2023

if run on master without any change, no that's not expected. Tests run fine on all our CI platforms. Maybe check that PROJ_NETWORK=ON is not set in your environment, as I suspect it could cause issues (just trying in my dev env, I see the same tests that fail for you fail for me when I define PROJ_NETWORK=ON)

Yep, that was it. Thanks. I was building based on a conda-forge environment and the PROJ in there must have turned that on.

test/gie/builtins.gie

The docs at the top of this file say it is autogenerated or at least partially. Should I still modify that or should I put stuff in more_builtins.gie? Is the idea of these files just a human-readable-ish language for common transformation operations?

@rouault
Copy link
Member

rouault commented Feb 2, 2023

Yep, that was it

filed as #3599

Should I still modify that or should I put stuff in more_builtins.gie?

ah yes, more_builtins.gie makes more sense

Is the idea of these files just a human-readable-ish language for common transformation operations?

short answer is yes I guess. More at https://proj.org/apps/gie.html

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

Successfully merging a pull request may close this issue.

2 participants