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 forward model for prisms #86

Merged
merged 58 commits into from
Sep 26, 2019
Merged

Add forward model for prisms #86

merged 58 commits into from
Sep 26, 2019

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Jul 30, 2019

Add forward model for prisms in Cartesian coordinates following
Nagy et al. (2000) and Nagy et al. (2002). Compute the potential and the
downward component of the gravitational acceleration generated by prisms
on computation points. Perform non-jitted checks for valid prisms
boundaries. Define a safe_atan2 function following Fukushima (2019) in
order to guarantee that the generated field satisfies Poisson's
equation. Use a safe_log function to allow the computation of
gravitational fields on singular points of the analytical solution.
Add test functions for symmetry and result comparison with an infinite
slab (Bouger plate).

Fixes #49

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.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

@santisoler
Copy link
Member Author

santisoler commented Jul 30, 2019

The test functions should include a few that check if the forward model has symmetry properties.

@santisoler
Copy link
Member Author

@leouieda I'm wondering if the top and bottom variables are not a little bit confusing: due to the fact that the vertical coordinate points downwards, the top boundary should be in fact lower than the bottom one.
What do you think? How should we design these variables?

@santisoler
Copy link
Member Author

I have some doubts about the atan2 function. What exactly is its expected behavior? What's the difference with numpy.arctan2?

@santisoler
Copy link
Member Author

@leouieda I'm wondering if the top and bottom variables are not a little bit confusing: due to the fact that the vertical coordinate points downwards, the top boundary should be in fact lower than the bottom one.
What do you think? How should we design these variables?

We agreed to change the vertical coordinate so it points upwards. See #89

@santisoler
Copy link
Member Author

I have some doubts about the atan2 function. What exactly is its expected behavior? What's the difference with numpy.arctan2?

The current atan2 function guarantees that the forward modelling has symmetry. If the numpy.arctan2 function is used instead of it, then the test_g_z_symmetry test function fails.
Would be nice to find out the reason for this.
While we do that, I think we should improve the docstring explaining these facts.

Remove then old minus sign. Because g_z is the downward component, we
want it pointing downwards.
Add test that compares the analytical solution of the g_z generated by
an infinite slab with the one generated by a large enough prism. Also
tests convergence: checks if the result gets closer to the analytical
solution if the horizontal dimensions of the prisms are increased.
It introduces some fixes to the equations that define the horizontal
components of the gravity acceleration.
Remove the set of points located around the center of the prism: g_z
must be zero on those points and testing their values is complicated.
Modify the safe_atan2 function in order to follow the Fukushima2019
definition. Add reference to Fukushima2019 on doc/references.rst.
Update test function for the new safe_atan2 function.
Add set of points located on the upward == 0 plane. The values of g_z on
those points must be all equal to zero.
@santisoler
Copy link
Member Author

@leouieda Besides the problem regarding the convergence test for very large prisms, I think this is looking really good, specially after implementing the safe_atan2() function defined by Fukushima (2019). Let me know what do you think.

@santisoler
Copy link
Member Author

One more thing I want to discuss is the fact that we are not allowing users to compute gravitational fields on computation points that falls inside the prism. Nevertheless, every paper claims that it can be safely done without modifying the analytical solution. Should we allow users to do so,? How do you think we should test it if so?

Remove the check that used to raise an error if the computation point
fell inside the prism. Add test function for g_z symmetry if computation
points fall inside the prism, and include computation points inside the
prism on the test function for potential symmetry.
santisoler and others added 7 commits September 16, 2019 11:02
Co-Authored-By: Leonardo Uieda <[email protected]>
Define a single set of computation points for the two examples. Modify
the size and location of the single prism. Improve how the gz results
are printed in order to avoid printing a numpy array. Create a title for
the two prisms example.
Redefine coordinates from a stacked numpy array to a tuple in order to
avoid unnecessary memory allocation. Fix Numba jitted function that
expected a numpy array.
Change order of lower and higher boundaries when checking if an error is
raised when invalid prism boundaries are passed. Remove unneeded lines.
Make use the already existing function to compute the gravity field of
an infinite slab instead of rewriting it.
@santisoler
Copy link
Member Author

Thanks for the review @leouieda and sorry for all the mistakes! Feel free to take a look at this when you have a spare time!

Add size of density array and number of prisms on the error message when
they mismatch in order to make clearer what's failing. The improvement
of such error messages was triggered after #98.
@santisoler
Copy link
Member Author

I've improved the error message for invalid density array in the same way I did on #98 and #104 .

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.

Nice work, Santi! Now our git history won't fail us in the future :)

@leouieda
Copy link
Member

Merging when CIs finish building against master

@leouieda
Copy link
Member

Just waiting for #105 to fix the linting errors

@santisoler
Copy link
Member Author

Thanks @leouieda !

Nice work, Santi! Now our git history won't fail us in the future :)

Haha, it's nice to see how we improved our way of doing things!

@santisoler santisoler merged commit 4f2dcbe into master Sep 26, 2019
@santisoler santisoler deleted the prism_gravity branch September 26, 2019 19:31
@santisoler santisoler mentioned this pull request Oct 5, 2020
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.

Prisms forward modelling
3 participants