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 mass, mean_density, and volume_equivalent_radius properties #173

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

MarkWieczorek
Copy link
Contributor

@MarkWieczorek MarkWieczorek commented Mar 25, 2024

This PR addresses the issue #164 concerning the addition of mass, mean_density, and volume_equivalent_radius properties to the ellipsoid classes. The following changes were made:

  • Created a top level file _constants.py to house to the gravitational constant G that is imported into the three ellipsoid classes.
  • Added the properties mass, mean_density, and volume_equivalent_radius to each of the three ellipsoid classes.
  • Clarified the docstrings of mean radius: This is, in fact, not the mean radius of the sphere (i.e., the degree 0 coefficient of the shape), but rather is the mean of the semi-axes (a + b + c)/3. This is defined as R_1 in WGS84 Moritz1988.

Comments

  • The mathematical equation for the volume_equivalent_radius is (self.volume * 3 / 4 / np.pi)**(1 / 3). If we expanded the definition of self.volume we could probably save a couple multiplications (but at the expense of readability).
  • I don't know if we need to say "Added for compatibility with pymap3d." in the Sphere docstrings. For me, these quantities are defined so that all classes take the same parameters.

And, I'm happy to modify anything to conform to your coding standards that are higher than mine!

Relevant issues/PRs:
Fixes #164

Copy link

welcome bot commented Mar 25, 2024

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

No matter what, we are really grateful that you put in the effort to do this!

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.

Thanks for this @MarkWieczorek! Merging it in.

boule/_sphere.py Outdated Show resolved Hide resolved
@leouieda
Copy link
Member

I agree that we don't need to worry about multiplying a few more times. This is not something called in a loop.

@leouieda leouieda merged commit 4439efb into fatiando:main Apr 15, 2024
15 checks passed
Copy link

welcome bot commented Apr 15, 2024

🎉 Congrats on merging your first pull request and welcome to the team! 🎉

If you would like to be added as a author on the Zenodo archive of the next release, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

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.

Add attributes: mass, mean_density, and volume_equivalent_radius
2 participants