Skip to content

Conversation

@mherrmann3
Copy link
Contributor

Addresses an error in the simplification of the latitude-to-longitude aspect calculation, see PR #110 (comment) and issue #109 (comment).

Sorry for the confusion. (@wsavran: but in this way, I can put it to the test what I just learned minutes ago in your git workshop 😄. Thanks again, Bill!)

(FYI: The whitespace clean-ups were made automatically by my editor)

Ultimately closes #109 (the part with the alternative/fast projection).

@billtron
Copy link

billtron commented Jun 8, 2021

This looks great! I'm going to merge as soon as the CI tests pass! Make sure to update your forked copy to get these latest changes. This will be included in the next release!

@codecov-commenter
Copy link

Codecov Report

Merging #125 (d3f0099) into master (5638e89) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   57.46%   57.46%           
=======================================
  Files          19       19           
  Lines        3120     3120           
  Branches      452      452           
=======================================
  Hits         1793     1793           
  Misses       1217     1217           
  Partials      110      110           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5638e89...d3f0099. Read the comment docs.

@wsavran wsavran merged commit 3a25969 into cseptesting:master Jun 8, 2021
@mherrmann3
Copy link
Contributor Author

Damnnnn. It's still not perfectly correct 😱 . The simplification in itself was proper, but the original suggestion is wrong. Let's recap:

LATKM = 110.574
lonkm = 111.320 * numpy.cos(numpy.deg2rad(lat_region))

So the y-to-x ratio (i.e., lat-to-lon ratio) is LATKM / lonkm, so:

ax.set_aspect(LATKM  / (111.320 * numpy.cos(numpy.deg2rad(lat_region))))

So simple; I feel embarrassed. At least the current implementation with this PR (and the original proposal) is off by only ~1%. But if we're going to use it, let's use it correctly.

To summarize:

I don't know why I had initially started with a wrong formula. 😕 Possible due to a trial-and-error approach of getting a good-looking aspect, instead of doing the maths.

@wsavran: I guess I cannot update this PR [see stackoverflow.com], so I'll need to open another PR, correct?

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

Successfully merging this pull request may close these issues.

Missing region border when specifying a projection; In addition: a (faster) alternative to using projections

4 participants