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

Put padding inside the transformation function #390

Open
LL-Geo opened this issue Mar 3, 2023 · 6 comments
Open

Put padding inside the transformation function #390

LL-Geo opened this issue Mar 3, 2023 · 6 comments
Labels
enhancement Idea or request for a new feature

Comments

@LL-Geo
Copy link
Member

LL-Geo commented Mar 3, 2023

Description of the desired feature:

Right now the padding process is done externally before passing it to the grid transformation
padding -> hm.filter -> unpadding
I'm thinking to put the padding and unpadding inside the transformation, so we can use one-line code to do the filter
where paddling and unpadding are handled inside the transformation.

Probably we can also work on different type of padding method, for example Fedi, 2012. But I guess this will be better to implement on the xrft side?
What do you guys think?

Are you willing to help implement and maintain this feature?
Happy to implement this, but also love to see contributors to tackle this one

@LL-Geo LL-Geo added the enhancement Idea or request for a new feature label Mar 3, 2023
@mdtanker
Copy link
Member

Yeah I think this would be a good addition. It is a little annoying having to remember the specifics for how to pad and unpad everytime.

Out of curiosity, have you tried any other padding options besides xrft? Such as xr.Dataarray.pad()? The xrft pad seems to work fine.

@LL-Geo
Copy link
Member Author

LL-Geo commented Mar 17, 2023

Yeah I think this would be a good addition. It is a little annoying having to remember the specifics for how to pad and unpad everytime.

Out of curiosity, have you tried any other padding options besides xrft? Such as xr.Dataarray.pad()? The xrft pad seems to work fine.

I haven't tried the xr.Dataarray.pad() yet, but I guess that would be similar. Yeah, Santi write the pad function in xrft, hahaha.

@leouieda
Copy link
Member

That's a +1 from me on this.

We had a bunch of padding options in the old fatiando here if anyone is curious: https://github.com/fatiando/fatiando/blob/master/fatiando/gridder/padding.py Most of those are covered by xarray already.

@santisoler
Copy link
Member

santisoler commented Mar 17, 2023

I agree too! It's annoying to having to handle the padding/unpadding steps every time we need to use these transformation functions.

Regarding the choice of xr.DataArray.pad() and xrft.pad(). They both use numpy.pad under the hood. The main difference is that the xarray method doesn't apply a consistent padding to the coordinates, something that we really need to do if we want to compute the FFT of the padded grid. That's why I wrote the xrft.pad() function. If you read its docstring you'll see that it offers all the padding methods that numpy.pad does (and by extension, the old fatiando padding functions).

In summary, I would make the apply_filter() function to handle the padding and unpadding steps before computing the fft and after computing the ifft, respectively. And I would use xrft.pad() for it.

Now we need to think how we would like to pass the options for the padding function.

One way could be to ask for a pad_kwargs dictionary. It introduces only one more argument, but it doesn't offer autocompletion. As an example, it will be use like this:

pad_kwargs = dict(
    pad_width={"easting": 50, "northing": 50},
    constant_values=0,
)

hm.upward_continuation(grid, height_displacement=500, pad_kwargs=pad_kwargs)

I think this decision is somewhat related to the idea that @mdtanker shared in #396. If we want the apply_filter() function to be smarter and be able to apply padding or to fill nans in the grid, then we should be clever about how we handle its arguments.

@matthewturk
Copy link

Heh, I wish I had had thought up that idea -- it's a good one! But I think you might mean @mdtanker ?

@santisoler
Copy link
Member

Heh, I wish I had had thought up that idea -- it's a good one! But I think you might mean @mdtanker ?

haha sorry! I just edited it. Reminder that I should actually type the handles instead of clicking the popup list (and click the wrong one...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Idea or request for a new feature
Projects
None yet
Development

No branches or pull requests

5 participants