-
Notifications
You must be signed in to change notification settings - Fork 68
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 #54
Conversation
The fastmath sacrifices accuracy for performance. We want to prioritize accuracy over performance.
@leouieda I drafted portions of the point mass forward modelling code. |
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.
Hi Santi, thanks for putting this together. I don't think we should be doing coordinate conversions in these functions. It's not much work for the user to convert coordinates before passing them in and makes sure they know what is going on. Plus, it's probably a bit wasteful to convert coordinates every time we call this function. Better to do it once outside of the function. This will probably be even more so when we do inversions.
Are you thinking of having this function calculate for multiple point masses? I'm not entirely sure how to handle this. We can probably optimize some things if we do this, like calculating sin and cos of the observation point latitude. But we also loose the simplicity of these functions. If the point masses have this, we should also do things this way for prisms, tesseroids, etc.
One advantage of leaving this as single element functions is that we can build other functions that compute on multiple elements and call these under the hood. For example, when calculating a regular grid of masses on a regular grid, there optimizations that we can make regarding symmetric points.
The main problem is that it's hard to do something generic that will work every case.
You're right, as I pointed on #46. I'll remove the coordinate conversion and assume spherical coordinates. |
When we build forward models we have two summations: one over the sources and one over the computation points. For example: for source in sources:
for point in computation_points:
result += forward_model(computation_point, source) If we leave this functions with single element computation we are speeding up the nested Did you consider this? Or you were thinking to Regarding the multiple elements implementation, I don't think it would be so hard. Instead of asking for a single point mass, the function could take a collection of them (in the same way the P.S: In my opinion, the forward models that we'll build should have the same logic, i.e. if one works with multiple elements, they all should also. |
@leouieda Can we continue discussing this PR so I can keep writing with a more precise North? |
When trying to get the test coverage, According to Numba documentation this can be done setting the environment variable |
Using parameters values equal to 1.0 can hide bugs in case the parameters are factors.
The implementation of this enhancement has been moved to #60 |
Add a forward modeling for point masses in geographic coordinates.
The coordinates of the computation points can be passed as list of arrays (
[longitude, latitude, height]
) while the point mass location can be specified as a list of its coordinates.The user can specify which coordinate system is using for these coordinates (geodetic or spherical geocentric).
The time demanding portions of the code have been decorated with Numba's
jit
.The
fastmath
argument is not used because it was proved to sacrifice accuracy over performance making test functions to fail.Fixes #47
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.