Check if normal gravity is computed on internal point

[MGomezN]

Fixes #44

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 and the base __init__.py file for the package.
• Write detailed docstrings for all functions/classes/methods. It often helps to design better code if you write the docstrings first.
• If adding new functionality, add an example to the docstring, gallery, and/or tutorials.
• Add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.

[MGomezN]

Hi! I am back. I just wanted to make a pull request for my last commit, but I couldn't erase the past commits. This time I tried to follow more carefully the Contributing Guidelines. I ran checks format, check, lint and test. All of them appear successful. However, I am dubious about the solution I implemented using the norm. Maybe is a condition that is always true by definition of "norm"... so, I'll wait for your review. Thank you!

[santisoler]

HI @MGomezN! Thanks again for your contributions.

I just wanted to make a pull request for my last commit, but I couldn't erase the past commits.

I'm not sure if I'm following you here. Have you tried to commit to your internal_point branch? In git we don't erase previous commits (unless it's strictly necessary) because that could potentially create a lot of troubles. Instead you can always add a new commit making changes that fixes the problems introduced by any previous commit. This not only makes version control easier, but also helps us to keep track of the changes and decisions we made in the past.

However, I am dubious about the solution I implemented using the norm. Maybe is a condition that is always true by definition of "norm"... so, I'll wait for your review.

I'm not entirely sure why are you checking if the norm of the height is greater than zero. Raising the warning if height < 0 should be enough, right?

On #66 you also added the same check for the Sphere class. Would you like to add it here if you want to continue the development of this feature on this PR?

[MGomezN]

HI @MGomezN! Thanks again for your contributions.

Thank you @santisoler

I'm not sure if I'm following you here. Have you tried to commit to your internal_point branch? In git, we don't erase previous commits (unless it's strictly necessary) because that could potentially create a lot of troubles. Instead you can always add a new commit making changes that fixes the problems introduced by any previous commit. This not only makes version control easier, but also helps us to keep track of the changes and decisions we made in the past.

No. I didn't try to commit to my internal_point branch. I worked with internal_point branch almost six months ago, and by that time, I had a lot of trouble with the environment, and also I made a reset of my PC. Therefore It seemed easier to start everything from zero last week when I resume my work on this issue. So, I closed my pull request related to the internal_point branch, and actually, I erased my fork of the Boule repository, and I started everything from the beginning: clone, fork, Install environment, and so on. I incorporated comments from Leonardo made on the internal_point branch, and as a result, I created commit 9b71364.

But… I forgot to run make format, make check, make linnet, so I did it. I discovered one error, I repaired that error, and then I created commit bd0dcd7.

I'm not entirely sure why are you checking if the norm of the height is greater than zero. Raising the warning if height < 0 should be enough, right?

Again, I forgot to ran the make test, I did it, and there was a new error saying height <0 is not ok because height could be an array, not just a scalar, so I tried to use the norm of the array “height,” and that was mi final commit a0a2239

So, I wanted to erase commits 9b71364 and bd0dcd7 because I know (now) they were incomplete. The most complete is commit a0a2239.
Jeje It seems I still don't get a good understanding of Git, but for me, it is unnecessary to leave 9b71364 and bd0dcd7 in the track of changes. That is why I said I wanted to delete them.

On #66 you also added the same check for the Sphere class. Would you like to add it here if you want to continue the development of this feature on this PR?

Ups, see? Not too much focus in my head. Yes, I want to change the Sphere class also.

[santisoler]

No. I didn't try to commit to my internal_point branch. I worked with internal_point branch almost six months ago, and by that time, I had a lot of trouble with the environment, and also I made a reset of my PC. Therefore It seemed easier to start everything from zero last week when I resume my work on this issue. So, I closed my pull request related to the internal_point branch, and actually, I erased my fork of the Boule repository, and I started everything from the beginning: clone, fork, Install environment, and so on. I incorporated comments from Leonardo made on the internal_point branch, and as a result, I created commit 9b71364.
But… I forgot to run make format, make check, make linnet, so I did it. I discovered one error, I repaired that error, and then I created commit bd0dcd7.

No big deal then! Just wanted to know if you were experiencing technical issues or if you willingly started again from scratch.

Again, I forgot to ran the make test, I did it, and there was a new error saying height <0 is not ok because height could be an array, not just a scalar, so I tried to use the norm of the array “height,” and that was mi final commit a0a2239

You are absolutely right! We cannot use height < 0 because height can be an array and it would return a boolean array, so the statement height < 0 cannot be evaluated as a single bool. Nevertheless, using np.linalg.norm would create always positive values, so it won't work for our purposes. What we need to check if any value of height is negative, so give np.any() a try.

So, I wanted to erase commits 9b71364 and bd0dcd7 because I know (now) they were incomplete. The most complete is commit a0a2239.
Jeje It seems I still don't get a good understanding of Git, but for me, it is unnecessary to leave 9b71364 and bd0dcd7 in the track of changes. That is why I said I wanted to delete them.

We virtually never delete a commit, as I said before, the best practice is to add a new commit that fixes the mistakes of the previous ones. Think of git as a journey: you know where you started and you know where you are right now, but you cannot erase all the points in the middle, because they took you where you are right now.
That being said, when we merge this branch into master we do a Squash and Merge: we squash every commit from this PR into a single one. So feel free to add as many commits as you want, they will not end up in the history of Boule's master branch.
Using Squash and Merge generates a way cleaner log history on master and makes the development of new features much easier by allowing us to break stuff on any other branch, we just need to be very careful what we merge to master and that's why we have all the CIs automatically checking stuff for us.

Ups, see? Not too much focus in my head. Yes, I want to change the Sphere class also.

No problem! Feel free to finish the warning on Ellipsoid and once you got it right, you can start adding it to Sphere.

[MGomezN]

You are absolutely right! We cannot use height < 0 because height can be an array and it would return a boolean array, so the statement height < 0 cannot be evaluated as a single bool. Nevertheless, using np.linalg.norm would create always positive values, so it won't work for our purposes. What we need to check if any value of height is negative, so give np.any() a try.

Hello @santisoler. I tried your suggestion in commit 8b48925

We virtually never delete a commit, as I said before, the best practice is to add a new commit that fixes the mistakes of the previous ones. Think of git as a journey: you know where you started and you know where you are right now, but you cannot erase all the points in the middle, because they took you where you are right now.
That being said, when we merge this branch into master we do a Squash and Merge: we squash every commit from this PR into a single one. So feel free to add as many commits as you want, they will not end up in the history of Boule's master branch.
Using Squash and Merge generates a way cleaner log history on master and makes the development of new features much easier by allowing us to break stuff on any other branch, we just need to be very careful what we merge to master and that's why we have all the CIs automatically checking stuff for us.

I understand. Looking forward to squash and merge all my mistakes jeje

No problem! Feel free to finish the warning on Ellipsoid and once you got it right, you can start adding it to Sphere.

I think it works now. I only feel insecure about the place where I wrote the warning (under de def of normal_gravity). I don't know if it must be written before, for example, after line 97, where are those bunch of checks e.g. @flattening.validator.

[santisoler]

I understand. Looking forward to squash and merge all my mistakes jeje

😂

I think it works now. I only feel insecure about the place where I wrote the warning (under de def of normal_gravity). I don't know if it must be written before, for example, after line 97, where are those bunch of checks e.g. @flattening.validator.

The warning is coded in the perfect spot. The requirement of height being positive or zero only applies to the normal_gravity method, so there's no point in creating a separate private method (like _check_flattening) for raising this particular warning: no other method will use it, and because the warning only takes ~3 lines, we are not cluttering the code.

Would you like to start writing a test function that checks if the warning is raised after passing a negative height?
If so, please create a new function in tests/test_ellipsoid.py. You can catch warnings in test functions with warnings.catch_warnings(record=True), like this:

boule/boule/tests/test_ellipsoid.py

Lines 59 to 67 in 145384d

	with warnings.catch_warnings(record=True) as warn:
	Ellipsoid(
	name="almost-zero-flattening",
	semimajor_axis=1,
	flattening=1e-8,
	geocentric_grav_const=1,
	angular_velocity=0,
	)
	assert len(warn) >= 1

If you need any help, please feel free to ask!

[MGomezN]

Would you like to start writing a test function that checks if the warning is raised after passing a negative height?
If so, please create a new function in tests/test_ellipsoid.py. You can catch warnings in test functions with warnings.catch_warnings(record=True), like this:

boule/boule/tests/test_ellipsoid.py

Lines 59 to 67 in 145384d

	with warnings.catch_warnings(record=True) as warn:
	Ellipsoid(
	name="almost-zero-flattening",
	semimajor_axis=1,
	flattening=1e-8,
	geocentric_grav_const=1,
	angular_velocity=0,
	)
	assert len(warn) >= 1

Thank you @santisoler . I added the test and the warning for boule.Sphere.normal_gravity() in commit 1f6f555. Fingers cross!

[santisoler]

This is looking awesome! I think we can merge it after a few minor changes.

I left a minor suggestion to remove the empty line on the method. Feel free to commit that suggestion (you can do that by pressing the Commit button on the comment).

Since you had to change the height < 0 check for the np.any(height < 0) in order to support height as an array, would you like to add a similar check on the same test function but now passing latitude and height as Numpy arrays?

I've also copied the TODO list on the PR description so we don't forget to do some things that are required before merging. I think you just need to add your name to the AUTHORS.md file in case you want your name to appear on every Zenodo release (this is optional). Read our AUTORSHIP.md file if you want to know more about how we handle authorship in Fatiando.

[leouieda]

Hi @MGomezN thank you so much for implementing this! I'm sorry this took so long. I just fixed a very minor typo in the warning message ("major" -> "greater than"; it's the same in portuguese so I always got these wrong as well 🙂).

Merging this in as soon as the CI tests pass!

Please feel free to open a new PR adding yourself to the AUTHORS.md file! You've more than earned it 🙂

[leouieda]

And a big thank you to @santisoler for providing brilliant guidance here (as always) 🎉

[welcome]

🎉🎉🎉 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, make sure that you have added your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository.

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.

[MGomezN]

Thanks, @santisoler @leouieda, for such an amazing demonstration of patience with me. I can't believe this pull request has been approved...

[leouieda]

Thank you for the patience with us (me mostly) 🙂 Again, I'm really sorry for letting this stall for so long.