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 area and area_equivalent_radius to all three ellipsoid classes #178

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

MarkWieczorek
Copy link
Contributor

@MarkWieczorek MarkWieczorek commented Mar 31, 2024

This PR adds the property area and area_equivalent_radius to the three ellipsoid classes. The "area_equivalent_radius" is what WGS84 calls $R_2$.

The equations for the area of bi-axial and tri-axial ellipsoids are taken from https://en.wikipedia.org/wiki/Ellipsoid#Surface_area. The equation for a bi-axial ellispoid is relatively simple, and only requires to use np.arctanh. The equation for a triaxial ellipsoid, however, requires computing two elliptic integrals. To do this, I have used the elliptic integral functions from scipy: scipy.special.elliprd and scipy.special.elliprf. This required adding scipy to the environment.yml and requirements-test.txt files.

I have verified that the area equivalent radius for WGS84 in boule is what is given in the literature. I have verified that the triaxial ellipsoid routine gives the correct value when a=b for WGS84.

If you don't like having scipy as a dependency, we could perhaps have this an an optional dependency, and check if it is available before running TriaxialEllipsoid.area (this is the only routine that requires it).

Relevant issues/PRs:

@MarkWieczorek
Copy link
Contributor Author

MarkWieczorek commented Mar 31, 2024

Please note that the python 3.7 tests are failing only because the scipy elliptic integral functions I used were introduced in version 1.8.0, and that version of scipy requires python 3.8+.

See https://docs.scipy.org/doc/scipy/reference/generated/scipy.special.elliprd.html#scipy.special.elliprd and https://docs.scipy.org/doc/scipy/release/1.8.0-notes.html.

@MarkWieczorek
Copy link
Contributor Author

Note: I have updated with github tests to use Python 3.8 and 3.12.

@leouieda
Copy link
Member

I opened #188 to drop 3.7 support than we can merge that into here once it's on main. Thanks!

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

@MarkWieczorek thanks for this! Just a minor change to add Scipy as a runtime dependency and then resolving the conflict with the workflow file. After that, I'll merge right away.

env/requirements-test.txt Outdated Show resolved Hide resolved
boule/_triaxialellipsoid.py Show resolved Hide resolved
@leouieda leouieda merged commit 1969c58 into fatiando:main Apr 17, 2024
15 checks passed
@leouieda
Copy link
Member

Merged! Many thanks for this!

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.

2 participants