-
Notifications
You must be signed in to change notification settings - Fork 69
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
Forward modelling for point masses in Cartesian coordiantes #71
Conversation
Add kernel functions for computing gravitational fields of point masses in Cartesian coordinates. Add coordinate_system argument to point_mass_gravity(). Add tests for the extended feature.
@leouieda Could you take a quick look at this. Specially to the design of the extended I added some functions to compute distances in Cartesian coordinates inside If you can't find time to review it, let me know. I think the main issues that can live here are related with design, so maybe I could merge it and we can discuss it afterwards when you have time. |
Looks good! It would be good to have a gallery example as well to visually verify that the results are correct. Maybe one with 2 masses at different depths with opposite densities? |
Thanks for the quick review @leouieda. I've added a simple example as you proposed. If you like it, feel free to merge this. |
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.
@santisoler really good! I like this new API. I made a few suggestions for the recipe.
harmonica/forward/point_mass.py
Outdated
def point_mass_gravity(coordinates, points, masses, field, dtype="float64"): | ||
def point_mass_gravity( | ||
coordinates, points, masses, field, coordinate_system="cartesian", dtype="float64" | ||
): | ||
""" | ||
Compute gravitational fields of a point mass on spherical coordinates. |
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.
It would be good to elaborate a bit more on the docstring (citation, definitions, equations [?]).
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.
Yes, absolutely! I tend to forget to do it because I know where I'm getting the equations from, but I really appreciate finding math on the docstrings when I try to use a new library or function.
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.
I added some math and references. I think we must make clearer that the third coordinate in Cartesian system is pointing downwards.
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.
The warning mentioned above should be enough for making it very clear.
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Increse depth difference of point masses in gallery example.
@leouieda Would you mind taking a final look at this? If you agree with the new variable name and don't have any other suggestion, I think this is ready to be merged. |
@santisoler this is great! I love that docstring! Made a few corrections to the text but once these are done feel free to merge! |
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Thanks for the review @leouieda! I like how it looks, so I'll merge it right away! |
Extend point_mass_gravity() to compute gravitational fields of point masses in Cartesian
coordinates. Add needed Numba jitted kernel functions in Cartesian coordinates. Add
coordinate_system argument to point_mass_gravity(). Add tests for the extended feature.
Fixes #48
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.