-
Notifications
You must be signed in to change notification settings - Fork 362
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
updated thresholds/values to work with PROJ 9.2 #2163
Conversation
This probably needs a check on PROJ(pyproj) versions within the tests to pass the minimum versions CI as well. |
ce0151a
to
28cabb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few minor comments to consider.
assert_almost_equal(np.array(eqdc.y_limits), | ||
(-10001965.729313632, 17558791.85156368), | ||
decimal=7) | ||
if pyproj.__proj_version__ >= '9.2.0': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts about changing the number of decimal places we compare to here? It seems like if we can compare to 2-3+ decimal places that should be good enough for Cartopy's purposes. We are really relying on proj for all the "official" transformations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any strong opinion about number of decimals. I do find it strange that cartopy supports different versions of proj that would lead to very different results (eg boundary of UK)
I would either not test values from proj at all, since they change between versions OR only support one version of proj that is considered "correct", most likely latest version of proj.
that's just my 2 cents no need to change anything for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with everything you say here :)
54f7541
to
cee33ee
Compare
i see tests are failing, but passing in my setup, I have an other look |
should be fixed now |
assert_almost_equal(np.array(eqdc.y_limits), | ||
(-10001965.729313632, 17558791.85156368), | ||
decimal=7) | ||
if pyproj.__proj_version__ >= '9.2.0': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with everything you say here :)
Rationale
fixes #2145
Implications
I cant speak for correctness of values
from release log https://proj.org/news.html#release-notes
it looks like EPSG and ESRI databases got updated so its probably related