Skip to content

Conversation

@joa-quim
Copy link
Member

@joa-quim joa-quim requested a review from a team March 10, 2025 12:10
@Esteban82
Copy link
Member

How could I test this? I try some examples from gravfft but I think I did something wrong.

@joa-quim
Copy link
Member Author

Honestly, I'm not sure how to test it with new numbers, but what I tested is that it doesn't break. We have quite a few tests for gravfft and all passed with this PR.

Regarding grdfft we actually do not have any tests this parameter (options -D and -I), but this one gives exactly the same thing before and after this PR

grdfft @geoid_1m.nc -Dg -V -Ggrav.nc

so I would say we re not breaking anything.

@Esteban82
Copy link
Member

I think that there is something missing.

gmt grdfft @geoid_1m.nc -Dg -V -Ggrav1.nc -M980619.9203
grdfft [ERROR]: Unrecognized option -M

And I think that I build the branch well because I can see the description of -M

gmt grdfft
...
  -M<mgal_at_45>
     Value for gravity in mGal at 45 degrees latitude. Default is 980619.9203 mGal (Moritz's 1980 IGF value).
...

@joa-quim
Copy link
Member Author

Yes, just submitted a commit to fix that. The thing is that grdfft is more convoluted in usage of this parameter and I had to parse the -M separately, but missed to make it be ignored on second parsing passage. Now all of these three give the same

grdfft @geoid_1m.nc -Dg -Ggrav.nc
build\src\grdfft @geoid_1m.nc -Dg -Ggrav.nc
build\src\grdfft @geoid_1m.nc -Dg -Ggrav.nc -M980619.9203

@Esteban82
Copy link
Member

Yes, now it works for me too.
I try these:

gmt grdfft @geoid_1m.nc -Dg -V -Ggrav0.nc
gmt grdfft @geoid_1m.nc -Dg -V -Ggrav1.nc -M980619.9203
gmt grdfft @geoid_1m.nc -Dg -V -Ggrav2.nc -M372635.5697

For the first two I got the same grid. For the last one a similar one but with different values.
1_map
2_map

I don't understand much about this subject, but it seems fine to me. I will approve it.

@Esteban82 Esteban82 added add-changelog Add PR to the changelog enhancement Improving an existing feature labels Mar 10, 2025
@joa-quim joa-quim merged commit 9ca3fc4 into master Mar 10, 2025
9 of 13 checks passed
@joa-quim joa-quim deleted the optional-mgal_at_45 branch March 10, 2025 18:31
joa-quim added a commit that referenced this pull request Mar 24, 2025
joa-quim added a commit that referenced this pull request Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add-changelog Add PR to the changelog enhancement Improving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants