-
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
Add total gradient amplitude transformation #478
Conversation
started adding the total gradient amplitude transformation.
- implemented the tga function in `transformations.py` - created the tga.py example - added the section Total gradient amplitude (aka Analytic Signal) to transformations.rst - added one test to test_transformations.py
💖 Thank you for opening your first pull request in this repository! 💖 A few things to keep in mind:
⭐ No matter what, we are really grateful that you put in the effort to do this! ⭐ |
Hum I just noticed that I removed the comments of |
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. |
* I (re)added the comments on the `derivative_northing_kernel` function * I also ran `make format` and `make check`
@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? |
Actually, there are 4 tests failing:
Those always failed for me. |
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.
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!
Co-authored-by: Santiago Soler <[email protected]>
Yes, those are failing because of the lines changed in |
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.
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 If you want, I can help you restore those lines in |
Thanks!
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. |
I (hope I) fixed `derivative_northing_kernel` in `_filters.py`.
This reverts commit 5b0155d.
I (hope I) fixed `derivative_northing_kernel` in `_filters.py`
Do the checks in CI run automatically? Or could I run it before I commit a change? Or can I run it somehow? |
* fixed comments on `total_gradient_amplitude` by moving to the section _Notes_ the TGA equation. * ajusted the TGA latex equation in `transformations.rst`
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.
Well, I hope I did it.
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. |
Also shrink the length of one of the lines to make flake8 happy.
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. |
Create a new function in harmonica/filters/_utils.py to run sanity checks on the passed grid. This function is now used by `apply_filter()` and by the new `total_gradient_amplitude()` function. This avoids repetition of code.
Add test functions that check if errors are being raised by the `total_gradient_amplitude` function when the passed grid is not valid.
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.
@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.
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). |
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
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. |
🎉 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 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. |
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? |
Right, that's a very good reason! Makes total sense.
It's preferred if you create a new one. The process would be:
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 |
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
_transformations.py
tga.py
exampletransformations.rst
total_gradient_amplitude_test
test totest_transformations.py
Notes:
transformations.rst
compiled correctly.