Add total gradient amplitude transformation

[leomiquelutti]

Add a new total_gradient_amplitude() function that computes the total gradient amplitude for a given 2D grid. It makes use of the derivative functions we already have in Harmonica, computing the horizontal ones through finite differences and the upward one through FFT. Add new gallery and tutorial examples, tests against a synthetic model, and list the new function in the API reference.

Relevant issues/PRs:

This PR resolves #470. I

• implemented the total gradient amplitude function in _transformations.py
• created the tga.py example
• added the section Total gradient amplitude (aka Analytic Signal) to transformations.rst
• added total_gradient_amplitude_test test to test_transformations.py

Notes:

• I had difficulty checking if the mathematical expression in the transformations.rst compiled correctly.
• I also don't know if I commented the function properly since I added a mathematical expression to it.

[welcome]

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

• Authorship Guidelines: Our rules for giving you credit for your contributions, including authorship on publications and Zenodo archives.
• Code of Conduct: How we expect people to interact in our projects.

⭐ No matter what, we are really grateful that you put in the effort to do this! ⭐

[leomiquelutti]

Hum I just noticed that I removed the comments of derivative_northing_kernel in filters\_filters.py. Do I have to undo it or can you refuse only this part of the proposed change?

[santisoler]

Thanks for opening this PR @leomiquelutti. I assigned myself to review it. I also started running CI so we it can test and check the style of your PR. Feel free to push fixes if you see any of those failing.

[leomiquelutti]

@santisoler I fixed the comments I accidentally removed + ran make format + check. It passed some tests that were falling before. Do I have to do something else?

[leomiquelutti]

Actually, there are 4 tests failing:

• test_derivative_northing_kernel[1]
• test_derivative_northing_kernel[2]
• test_derivative_northing_kernel[3]
• test_laplace_fft

Those always failed for me.

[santisoler]

Thanks a lot @leomiquelutti for all these changes, they are greatly appreciated!

I just went through it, this PR is in very good shape. I just added a few comments and suggestions below. Feel free to commit them if you like them, or let me know if you would approach in a different way.

I've also pushed a change adding the new function to docs/api/index.rst, so it's listed in the API Reference in the docs.

One thing that we are not documenting properly is that we are not adding new gallery examples, just tutorials. The reason behind it is because we found tutorials way more educational than isolated examples (which need to have a meaningful plot that can make users relate to them and click them). I think your tutorial is looking awesome, so how would you feel about dropping the example? Let me know what do you think!

[santisoler]

Actually, there are 4 tests failing:

* test_derivative_northing_kernel[1]

* test_derivative_northing_kernel[2]

* test_derivative_northing_kernel[3]

* test_laplace_fft

Those always failed for me.

Yes, those are failing because of the lines changed in derivative_northing_kernel. See one of my comments above.

[leomiquelutti]

Your corrections and suggestions are all good. I am only confused whether I should resolve the conversations or not. And what else I should do, actually.

[santisoler]

Your corrections and suggestions are all good. I am only confused whether I should resolve the conversations or not. And what else I should do, actually.

Great! I saw you already merged a few of those, awesome!

I don't think there's a correct etiquette for marking conversations as resolved. When I usually go through someone else's review on my code, I mark them as resolve as I apply the suggested changes without any particular comment. If I took another approach, disagree with the suggestion or ask for more clarification I leave them as unresolved. Basically I used the resolve/unresolved state as a reminder of what things need to be done before merging. But that's up to you.

So, for a PR to be merged, it needs to pass all checks in CI, along with an approved review from one of the developers or maintainers. In this case, I'm seeing some of the checks failing, and we still have those modified lines in derivative_northing_kernel that should be restored (otherwise the tests on that function will keep failing). I also marked a few conversations as unresolved, since I think they need to be addressed.

If you want, I can help you restore those lines in derivative_northing_kernel, no worries.

[leomiquelutti]

I don't think there's a correct etiquette for marking conversations as resolved. When I usually go through someone else's review on my code, I mark them as resolve as I apply the suggested changes without any particular comment. If I took another approach, disagree with the suggestion or ask for more clarification I leave them as unresolved. Basically I used the resolve/unresolved state as a reminder of what things need to be done before merging. But that's up to you.

Thanks!

If you want, I can help you restore those lines in derivative_northing_kernel, no worries.

Now I think I got it @santisoler! The file no longer appears as changed (as it should not). If I cannot make this time, I'll leave it to you if you are willing to help me 😃

And I'll go and take a look at the unresolved comments to address them.

[leomiquelutti]

So, for a PR to be merged, it needs to pass all checks in CI, along with an approved review from one of the developers or maintainers. In this case, I'm seeing some of the checks failing, and we still have those modified lines in derivative_northing_kernel that should be restored (otherwise the tests on that function will keep failing). I also marked a few conversations as unresolved, since I think they need to be addressed.

Do the checks in CI run automatically? Or could I run it before I commit a change? Or can I run it somehow?

[leomiquelutti]

Well, I hope I did it.

[santisoler]

So, for a PR to be merged, it needs to pass all checks in CI, along with an approved review from one of the developers or maintainers. In this case, I'm seeing some of the checks failing, and we still have those modified lines in derivative_northing_kernel that should be restored (otherwise the tests on that function will keep failing). I also marked a few conversations as unresolved, since I think they need to be addressed.

Do the checks in CI run automatically? Or could I run it before I commit a change? Or can I run it somehow?

They usually run automatically after you push a change. But I think GitHub has strengthen their security and maybe because you're a new contributor I need to approve the run of CI manually. This is annoying... but after we merge this PR, you won't face this issue again.

[santisoler]

Indeed, we have the "Require approval for first-time contributors" active in the configuration of GitHub Actions. Here are some docs about it.

I thought that once I approve the first run, then every run will be approved. But apparently not, I need to go and manually approve every run.

[santisoler]

@leomiquelutti I just pushed a few minor changes. I saw that you added some checks to the new function that raise errors when the passed grid is not valid (not 2D or if they contain nans). Thanks for that! I just took the lines from apply_filter that run those checks and turned them into their own function. Now the total_gradient_amplitude function uses that function instead of repeating code. I also added a few tests to check if the function raises errors on an invalid grid, basically to achieve higher test coverage. And a minor adjustment to the LaTeX equation in the user guide.

This is looking great! If CI passes, I think this is ready to be merged.

[leomiquelutti]

Hi @santisoler is there anything else I should do about this PR? :)

I am only waiting for this PR to be done to open a new one to implement the tilt (if you mantainers agree to it, of course).

[santisoler]

Hi @santisoler is there anything else I should do about this PR? :)

Hi @leomiquelutti! No, nothing on your side. I just need to merge it. I'll do it today after CI finishes (I just updated this branch with the latest changes in main).

I am only waiting for this PR to be done to open a new one to implement the tilt (if you mantainers agree to it, of course).

That would be great! Is there a reason why you were waiting for this one to be finished? In most cases you can work on several features in parallel (unless they need a particular bit of code that is being developed in another PR). There's no particular rush to have these two transformations, but I was just curious about this comment.

[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, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

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.

[leomiquelutti]

That would be great! Is there a reason why you were waiting for this one to be finished? In most cases you can work on several features in parallel (unless they need a particular bit of code that is being developed in another PR). There's no particular rush to have these two transformations, but I was just curious about this comment.

This was my first experience contributing to an open-source project, so I wanted to see that everything went well before starting a new task.

A question @santisoler: should I keep working in this same branch or create a new one?

[santisoler]

This was my first experience contributing to an open-source project, so I wanted to see that everything went well before starting a new task.

Right, that's a very good reason! Makes total sense.

A question @santisoler: should I keep working in this same branch or create a new one?

It's preferred if you create a new one. The process would be:

1. Switch to main in your local repo.
2. Pull changes from the main branch in fatiando/harmonica.
3. Create a new branch where you'll start working on the new feature.
4. Open the PR for merging that new branch into main.

We do this for every PR, specially when you're working on different things in parallel, you don't want to mix changes from different PRs in a same branch. For example, you can now create one branch for the tilt implementation, and another branch to add yourself tot he AUTHORS.md file 🙂 .