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

Fix limits/threshold of projections with non-default Globe #1607

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jul 12, 2020

Rationale

Some limits and thresholds are hard-coded, but this causes incorrect projections when the Globe radius is much smaller than the default. Either the map is tiny in very large limits, or lines are not projected correctly.

Implications

TransverseMercator is no longer tiny when using a small Globe radius; other projections do not cut projected lines randomly.

@dopplershift
Copy link
Contributor

Code changes look good in general. Looks like this broke the test_grid_labels test in test_gridliner.py, and angered Travis by trying to generate some HTML with the result image base64-encoded.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 14, 2020

Weird, I thought I saw that failure before making this change and thought it was unrelated, but I guess not.

It formerly did not scale with Globe radius, and so would produce a very
very small result within a large boundary if using any smaller Globe radius.
Fixes SciTools#1572, testing the example in the other projections as well.
Using the example from SciTools#1572 shows that the lines are jagged and the
threshold is too high.
@QuLogic
Copy link
Member Author

QuLogic commented Aug 6, 2020

The test failure is because the AzimuthalEquidistant is now missing a whole bunch of lines. I rebased hoping that your 1000-point PR would have fixed it, but it appears not.
image

@QuLogic QuLogic modified the milestones: 0.19, 0.20 Apr 22, 2021
@dopplershift dopplershift modified the milestones: 0.20, 0.21 Sep 17, 2021
@QuLogic QuLogic modified the milestones: 0.21, 0.22 Sep 11, 2022
@dopplershift dopplershift modified the milestones: 0.22, Next Release Aug 4, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and will help folks when dealing with their moon/astro projections too. Just needs a rebase/update I think.

@@ -841,9 +841,17 @@ def __init__(self, central_longitude=0.0, central_latitude=0.0,
proj4_params += [('approx', None)]
super().__init__(proj4_params, globe=globe)

# TODO: Let the globe return the semimajor axis always.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this would be nice.

Comment on lines +845 to +846
a = np.float(self.globe.semimajor_axis or WGS84_SEMIMAJOR_AXIS)
b = np.float(self.globe.semiminor_axis or a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for these to be np.float() instead of just leaving them as the values that are returned?

@@ -21,16 +22,56 @@ def setup_class(self):
self.point_b = (0.5, 50.5)
self.src_crs = ccrs.PlateCarree()

def check_args(self, approx, proj, other_args):
if ccrs.PROJ4_VERSION < (6, 0, 0):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can get rid of this check now since we are dependent on pyproj's PROJ version and looks like we are at 8+
https://pyproj4.github.io/pyproj/3.3.1/

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

Successfully merging this pull request may close these issues.

4 participants