Add total gradient amplitude transformation

leomiquelutti commented 4 months ago

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

[x] implemented the total gradient amplitude function in _transformations.py
[x] created the tga.py example
[x] added the section Total gradient amplitude (aka Analytic Signal) to transformations.rst
[x] 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[bot] commented 4 months ago

💖 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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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.

santisoler commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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?

santisoler commented 3 months ago

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 commented 3 months ago

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.

leomiquelutti commented 3 months ago

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 commented 3 months ago

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[bot] commented 3 months ago

🎉 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 commented 3 months ago

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 commented 3 months ago

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:

Switch to main in your local repo.
Pull changes from the main branch in fatiando/harmonica.
Create a new branch where you'll start working on the new feature.
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 🙂 .

fatiando / harmonica

Add total gradient amplitude transformation #478