WIP Add horizontal Cartesian components for point masses

[birocoles]

Implement horizontal Cartesian components g_northingand g_easting for point masses. This PR addresses some of #85 . Components in spherical coordinate system will be implemented in the future.

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.

[welcome]

💖 Thanks for opening this pull request! 💖

Please make sure you read our Contributing Guide and abide by our Code of Conduct.

A few things to keep in mind:

• Remember to run make format to make sure your code follows our style guide.
• If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
• All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
• No matter what, we are really grateful that you put in the effort to do this! ⭐

[birocoles]

Hi @santisoler ,

Please take a look at this PR and give me a feedback. I didn't include the routines in the doc/api/index.rst nor create a tutorial yet.

[santisoler]

Thanks @birocoles for tackling this down! It looks very nice and I'm sure this will be merged very quickly.

I leave some comments bellow.

Besides, I think it would be nice to expand the math on the docstring by adding how we define these two extra acceleration components.

[birocoles]

I have just fixed the problem with numerical derivatives. There is still a problem related to the unused distance_spherical imported from utils at the begining of point_mass.py.

[birocoles]

Hi @santisoler and @leouieda , I have removed the unused distance_spherical function from point_mass_gravity.py. The functions computing things in spherical coordinates use the function distance_spherical_core. Besides, I have included the components g_northing, g_easting and g_upward in the documentation. I opted for using g_upward instead of g_up for convenience. No problem if you prefer a short name instead. I have also replaced the terms gravity by gravitational.

[santisoler]

This is looking really good @birocoles! Nice work! 💪

I like the g_{upward} notation more than the short but not so explicit g_{up}. Moreover, it's in agreement with how we note the upward components on the code.

I like the acceleration tests against potential values. Although I'm a little bit troubled about the expected accuracy of the numerical approximation of the acceleration components by finite difference. I've seen you set a delta (finite difference) equal to 0.1 meters and a distance from point mass to each computation point of approximately 500 meters. When you check if the numerical value is almost equal to the analytical one, npt.assert_allclose assumes a relative difference of 1e-7 by default. Do we have a way to bound the expected error to such tolerance? @leouieda do you have some thoughts on this?

Finally, I left some minor thoughts and suggestions on inline comments.

[birocoles]

@santisoler ,

I have used the central finite difference formula for computing the approximated acceleration components in the tests test_potential_versus_g_easting, test_potential_versus_g_northing and test_potential_versus_g_z. In this case, the truncation error can be computed by using the classical formula for central finite difference. According to this formula, the truncation error is given by:

We could use this formula. What do you think?

[santisoler]

That kind of error estimation is what I meant. Although I have my doubts about applying it as it is. We always struggle with how to tests analytical computations. On this case, computing the third derivative of our target field is even more complex than computing the field itself, so if we made a mistake on the computation of the field, it's very likely we would make the same mistake on the test function, resulting on a PASSED test although the error is hidden in there.

I'm thinking how to avoid computing this derivative. What about trying to bound the relative error instead of checking the result against the absolute error? For example, if we take the comparison of g_z against the potential, its relative error could be computed as:

If we assume that the epsilon is the same computation point as r, then the relative error is:

This expression for the relative error is not that complex, and we would only need to use the distance_cartesian function.

What do you think? I know there are a lot of simplifications on these calculations, but I don't think the test functions should be way more complex than the function we want to test.

[birocoles]

Hi @santiago, it is ok for me. I will implement this today.

[leouieda]

Hi @santiago, it is ok for me. I will implement this today.

Nothing like some vacation time to get coding done 😉

[leouieda]

@birocoles I think some of the changes here would be better suited for separate PRs. In particular, the spherical distance bug fix and replacing "gravity" with "gravitational" (each in a separate PR). That will help us keep the history clean and we can get those merged in quicker than this PR (which is getting big and taking a long time).

[birocoles]

ok @leouieda, no problems for me. But I don’t know how to split this PR 😅

[leouieda]

@birocoles that depends on how well you curated you git commits :) I'll see if I can separate them without much work. Otherwise, we can keep this as is. It's a lot easier to separate things from the start.

[leouieda]

Right, the changes are all mixed together in the same commits. So there is no way to separate renaming things to gravitational and the bug fix from the other changes. This is one reason to remember to commit often and keep changes separate for commits. We're all guilty of doing this but in situations like this is when we're screwed for not having it.

Another thing you could do is create a new branch and manually add the bug fix (using the right distance function). We could do it for you but I want you to have credit for having found it and fixed it.

[birocoles]

I will split things into three parts: 1) gravity to gravitational, 2) function distance and 3) all the remaining stuff. Ok?

[leouieda]

👍 But if you're short on time, don't worry about it this time. Just keep it in mind for the next PR

[leouieda]

@birocoles I'm merging in the latest changes from #116 and #117 here and resolving the conflicts.

[leouieda]

Done! Thanks for all your help with this! What is left to do here? Are we just waiting for the finite difference tests? If so, I'd suggest merging this in and implementing those in a separate PR as they will require some experimentation still (and this PR is getting big and out of hand).

[birocoles]

Sorry @leouieda , I got confused here. I have created another branch with the new functions for computing g_northing and g_easting, as well as the related tests and documentation modifications. I thought you would shut this PR down. I have just reviewed the new branch for creating a new PR. Do you prefer to take a look in the new PR or preceed with this one?

[leouieda]

@birocoles oh sorry for the confusion! Its up to you want to use this one or create a new one. There shouldn't be any difference. Did you make any changes there that aren't included in this branch?

[birocoles]

I think the things are more organized there. I prefer to create a new PR and close the this one.

[leouieda]

👍 whatever is best for you. Closing this in favor of #119