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

Fix coordinate rounding errors in FFT #398

Merged
merged 9 commits into from
Jul 6, 2023

Conversation

mdtanker
Copy link
Member

Here is a quick fix for the rounding error of the coordinate values during the FFTs. I've simply copied the original grid coordinates back to the grid after filtering.

This now allows users to add the filtered results as a new variable to the dataset/dataarray:

magnetic_grid_padded["upward_continued"] = hm.upward_continuation(magnetic_grid_padded, height_displacement=500)

Relevant issues/PRs:

Fixes #395

@mdtanker
Copy link
Member Author

mdtanker commented Apr 3, 2023

@santisoler this is ready for review/merge by the way 👍. Not sure if there's some button or something I'm meant to click to say it's ready to go.

@santisoler
Copy link
Member

Thanks @mdtanker for this!!

I think it perfectly fits the bug. The only thing I would add is a test function that checks that the coordinates of the transformed grid are the same as the ones in the original one. This test should fail if the patch is removed. Would you like to add such test?

@mdtanker
Copy link
Member Author

mdtanker commented Jul 4, 2023

Alright, the test is added! It passes as is, and fails if you comment out the fix in the apply_filter().

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 a lot @matthewturk for adding the test. This is ready to be merged. I just made two small suggestions. I'll commit them and merge this PR. 🚀

harmonica/tests/test_filters.py Outdated Show resolved Hide resolved
harmonica/tests/test_filters.py Outdated Show resolved Hide resolved
@mdtanker
Copy link
Member Author

mdtanker commented Jul 5, 2023

@santisoler good catches. Thanks for checking that.

@santisoler santisoler merged commit cf89138 into fatiando:main Jul 6, 2023
20 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.

Coordinates decimal issue in filters
2 participants