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

Add a method for the ellipsoid geocentric radius #37

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Add a method for the ellipsoid geocentric radius #37

merged 2 commits into from
Jul 10, 2020

Conversation

leouieda
Copy link
Member

This is the distance from the center of the ellipsoid to its surface as
a function of geodetic latitude.

Fixes #34

Reminders:

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst and the base __init__.py file for the package.
  • Write detailed docstrings for all functions/classes/methods. It often helps to design better code if you write the docstrings first.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.
  • Add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.

This is the distance from the center of the ellipsoid to its surface as
a function of geodetic latitude.
@leouieda leouieda requested a review from santisoler July 10, 2020 10:12
@leouieda
Copy link
Member Author

@MarkWieczorek just to check, this is what you had in mind right?

@MarkWieczorek
Copy link
Contributor

That is about what I was looking for: But would it be possible to allow inputting geodetic or geocentric latitude? Perhaps you could just add a default parameter like geodetic=True. If False, the geocentric latitude would first be converted to geodetic.

I have almost never used geodetic coordinates, and when dealing with spherical harmonics, it is natural to use the gecentric latitude :)

@leouieda
Copy link
Member Author

Hum, I'll need to think about this a bit since we need the radius in the first place to convert geocentric to geodetic. So it doesn't seem to be straight forward. But there should be a way.

@leouieda
Copy link
Member Author

Alright, I worked out the equation and implemented the option for geodetic=False and some tests. I added a note saying that this is an exception since we generally favor solutions where the conversions are done by the user prior to input to simplify the code. But this wouldn't be an option here since there is no way to do the conversion without knowing the radius already.

Seems like I got my math right 🙂

Copy link
Member

@santisoler santisoler 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 great @leouieda!

I understand why we are making the exception of making conversions within the function.
Although it's no desirable, for this specific case it's will not only run faster, but we might also prevent some possible mistakes by the user when trying to apply coordinate conversions to get the geocentric radius in from the spherical latitude.

@leouieda leouieda merged commit 2df7879 into master Jul 10, 2020
@leouieda leouieda deleted the radius branch July 10, 2020 14:07
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.

Provide function that returns the radius as a function of latitude
3 participants