Pad grids automatically before applying transformations#608
Pad grids automatically before applying transformations#608santisoler merged 23 commits intomainfrom
Conversation
Changes how keyword arguments are handled by this function since we now need one set for the padding and one for the filter.
They are failing but I don't know why yet
We have a better test now against a synthetic and this test didn't work very well before, since it required removing a mean from the Oasis result for some unspecified reason.
Was using the potential but it's too smooth and causes problems at the edges. Switch to gz instead and it works when looking at a smaller region inside the area.
leouieda
force-pushed
the
padding-inside
branch
from
5fd8ce8 to
2f886f6
Compare
|
To do:
|
Introduce an argument to apply_filter for dropping coordinates. We only really need to do this for upward continuation. The other filters don't alter the location of points. Update the filter tests to use synthetics instead of Montaj results.
|
@santisoler @mdtanker I'm done here and could use a quick review. The transformations are much better now and so are the tests. @mdtanker I ended up sticking with the "edge" mode since it seemed to work better in the tests and docs than the "reflect" mode. Not sure why. |
santisoler
left a comment
santisoler
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward, @leouieda!
I think it's looking great. I left a few suggestions to follow numpydoc style for optional arguments. I just went with , optional, but we can also use default: ... if you like it better.
I also added a few suggestions to force to pass optional arguments as keyword arguments. I know that would break backward compatibility, but I don't think a lot of users are actually using them, and the change wouldn't be too hard to update on their side. Let me know what do you think.
Feel free to merge afterwards!
mdtanker
left a comment
mdtanker
left a comment
There was a problem hiding this comment.
These changes look good to me! The only suggestions I have are about the default pad width. The docstrings could be interpreted as the default being 25% of each dimension, meaning a different pad width is used for each dimension, whereas the code uses 25% of the larger of the two dimensions. I added commit suggestions to clarify the docstrings. The other option would be actually to use pad widths per dimension. Since most study regions are typically somewhat near square, this is probably not a big issue, but with a very elongate region, maybe a default of 25% of each dimension's length would be better. I'm happy with either!
There was a problem hiding this comment.
These changes look good to me! The only suggestions I have are about the default pad width. The docstrings could be interpreted as the default being 25% of each dimension, meaning a different pad width is used for each dimension, whereas the code uses 25% of the larger of the two dimensions. I added commit suggestions to clarify the docstrings. The other option would be actually to use pad widths per dimension. Since most study regions are typically somewhat near square, this is probably not a big issue, but with a very elongate region, maybe a default of 25% of each dimension's length would be better. I'm happy with either!
Sorry, still figuring out how this review system works, see comment above.
Make arguments keyword only. Co-authored-by: Santiago Soler <santisoler@fastmail.com>
It would cause problems for very asymmetric grids.
|
Thanks, @santisoler and @mdtanker! @santisoler I made all arguments keyword only. I think it's OK. The errors aren't difficult to fix when people encounter them. And I always forget the "optional" in the description. @mdtanker good catch! I opted to make it 25% of each dimension as is mentioned in the docstring. You have a good point about grids which are highly asymmetric being a problem with the previous code. Thanks! |
Add `optional` in type description of a few parameters in the modified functions. Co-authored-by: Santiago Soler <santisoler@fastmail.com>
3 participants
Rework the
apply_filterfunction to make it apply and then remove padding when applying a filter automatically. The padding can be turned off or controlled by passing keyword arguments. The default padding mode is "edge" which works well with our tests and examples. Dropping non-dimensional coordinates is now handled byapply_filteras well instead of the FFT functions. Only drop non-dimensional coordinates on upward continuation since that's the only transformation that changes coordinates. Update the tests for transformations, in particular the tilt, Gaussian filters, and reduction to the pole which now test against a synthetic instead of the Montaj results from previously. Remove the unused Montaj testing grid.Relevant issues/PRs: Fixes #390 and fixes #630