-
Notifications
You must be signed in to change notification settings - Fork 30
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
Lambert projection #214
Lambert projection #214
Conversation
Keeping up to date.
… styles with empty class methods.
… which path to take still TODO.
…jection.project to a class method. Might not be ok as is!!
…ection.project to a class method. Might not be ok as is!!
…he method looks completed (not tested)
…the method looks completed (not tested)
This is a great addition to the projection capabilities, thanks for this @friedkitteh! Good catch with the ".py" endings in Let me know when you need feedback. |
Ok, finally! I think I am pretty happy with how it turned out now - it is (imo) a lot more compact, neat and clear and should be ready for review! 👍 |
I can also add that it passed all 8 tests in 0.15s, I did not want to wake Travis for now.. |
Updated methods to follow PEP8 Swapped some test methods with numpy.allclose(). Pytest.approx() still used mostly due to simplicity. [skip ci]
[skip ci]
Numpy.where() Ordered by: standard name ncalls tottime percall cumtime percall filename:lineno(function) For-loop Ordered by: standard name ncalls tottime percall cumtime percall filename:lineno(function) ############################################################################################## Numpy.where() Ordered by: standard name ncalls tottime percall cumtime percall filename:lineno(function) For-loop Ordered by: standard name ncalls tottime percall cumtime percall filename:lineno(function) |
Great that you timed it! Is this ready for review? I think you should be allowed to request a review from me under the "Reviewers" part of the sidebar? |
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.
Nice implementation of the Lambert projection from Callahan and De Graef (2013)! This will be used a lot. Also thankful for your fixes of other typos in the other projections.
I have three suggestions for improvements: adding a reference to the above mentioned paper, a change which might save some microseconds, and a correction of a "syntax wording". See more detailed comments below.
I have in addition some minor style comments which are unimportant and you can do with as you see fit.
After this we'll merge and I will set up a "Projections" user guide page, which you might want to add to later if you want, or I can do it if not.
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@friedkitteh, I just added a commit, so you need to pull this commit before pushing I think. I added your full name in the credits, please change if you want. |
…ikuchipy into lambert_projection
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
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.
Great! Will merge after Travis passes all builds.
Congratulations on, and thanks a lot for, your first contribution, @friedkitteh! 🎊 |
Description of the change
Continuation of the Gnomonic and Spherical projections already implemented.
Allows for the transformation of a vector from Cartesian coordiantes to the Lambert projection.
Also allows for transformation both ways between Gnomonic and Lambert projections.
Progress of the PR
Minimal example of the bug fix or new feature
For reviewers
later.
__init__.py
.the unreleased section in
doc/changelog.rst
.