Skip to content
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 new tilt_angle transformation function #486

Merged
merged 26 commits into from
Aug 12, 2024

Conversation

leomiquelutti
Copy link
Contributor

@leomiquelutti leomiquelutti commented Mar 29, 2024

Add new tilt_angle transformation function that computes the tilt from a given a grid of potential field data. Add the function to the API index. Include tests and an example for the new function.

Changes proposed:

  • added tilt function to _transformations.py
  • added it to the index.rst
  • created an example in tilt.py
  • added it to __init__.py
  • added the class TestTilt in test_transformations.py

Some points/questions:

  • The TestTilt::test_against_synthetic is failing and I don't know why.
  • Maybe the plots in tilt.py require a title. I added but had a hard time with pyGMT, so I left them commented.
  • I think we should merge the examples, maybe all in one .py file, comparing the transformations from TMI to the ones from RTP TMI. But that should be done in another PR.
  • Do I add it to transformations.rst?

Can you please help me @santisoler.

* added `tilt` function to `_transformations.py`
* added it to the `index.rst`
* created an example in `tilt.py`
* added it to `__init__.py`
* added the class `TestTilt` in `test_transformations.py`; however, the test is failling.
For some reason that I don't know the test of tilt passes when you change the sinal of g_z in `TestTilt:test_against_synthetic` in `test_transformations.py`
Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @leomiquelutti! Made a few comments but nothing major. Good work!

harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Show resolved Hide resolved
harmonica/tests/test_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
leomiquelutti and others added 3 commits April 15, 2024 17:42
* changed from `tilt` to `tilt_angle`
* added Miller and Sing 1994 reference
@leomiquelutti
Copy link
Contributor Author

Do you know why the test for macOS for python 3.8 is failing and/or how to solve it, @leouieda?

@santisoler
Copy link
Member

It was due to a failure when trying to push the coverage report to Codecov. I reran it and now it's passing. Nothing wrong with your code ❤️

Copy link
Contributor Author

@leomiquelutti leomiquelutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, is there anything else to do here? 🙂

Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leomiquelutti for the delay. I went through the PR and left a few comments below. Nothing critical, just some suggestions. Let me know what do you think, and feel free to ask for help, I can apply some changes myself if you are too busy.

I think we can squeeze this PR in the next Harmonica release! 🚀 Thanks a lot for your contribution!

harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/tests/test_transformations.py Outdated Show resolved Hide resolved
examples/transformations/tilt.py Outdated Show resolved Hide resolved
grid=magnetic_grid,
projection="X?",
cmap=True,
# frame=["a", "+tMagnetic anomaly intensity (TMI)"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add a title here. Otherwise, we should remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @santisoler,

I left the frames (titles) with comments because I could not add titles to the images in a way that would look good. Right now I am struggling with the installation of pygmt (again, I always suffer with that under Windows) to try to solve it.

If you would like to address it, I encourage you 😁. Otherwise, I will probably remove them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great. No worries. I applied a few changes to the plots so they look better. Thanks for adding the example though!

grid=rtp_grid,
projection="X?",
cmap=True,
# frame=["a", "+tTMI reduced to the pole (RTP)"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this one

grid=tilt_grid,
projection="X?",
cmap=True,
# frame=["a", "+tTilt from TMI"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this one.

grid=tilt_rtp_grid,
projection="X?",
cmap=True,
# frame=["a", "+tTilt from RTP"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one.

@santisoler santisoler added this to the v0.7.0 milestone Jun 28, 2024
Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leomiquelutti for applying these changes. I just left a few suggestions I'm going to merge right away. I'm also going to add the titles to the pygmt figures shortly.

I'm planning to merge this today so we can have it before I make the release.

harmonica/_transformations.py Outdated Show resolved Hide resolved
harmonica/_transformations.py Outdated Show resolved Hide resolved
examples/transformations/tilt.py Outdated Show resolved Hide resolved
examples/transformations/tilt.py Outdated Show resolved Hide resolved
@santisoler santisoler dismissed leouieda’s stale review August 12, 2024 17:59

All comments that @leouieda made were addressed.

@santisoler santisoler changed the title Add tilt transformation Add new tilt_angle transformation function Aug 12, 2024
@santisoler santisoler merged commit c001529 into fatiando:main Aug 12, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants