Skip to content

Implement our own FFT functions for xr.DataArray - #677

Open
santisoler wants to merge 20 commits into
mainfrom
fft
Open

Implement our own FFT functions for xr.DataArray#677
santisoler wants to merge 20 commits into
mainfrom
fft

Conversation

@santisoler

@santisoler santisoler commented Apr 27, 2026

Copy link
Copy Markdown
Member

Add public harmonica.filters.fft and harmonica.filters.ifft functions to compute the FFT and its inverse of a xr.DataArray, respectively. Remove xrft as a dependency, but keep it as a requirement to run the tests. Add harmonica.filters.pad and harmonica.filters.unpad functions as modified versions of the ones in xrft. Add tests for the new functions: the FFT functions are tested against their xrft counterparts, while the tests for the padding functions that were already existing in xrft were included. Skip doctests for some padding functions since Xarray changed the order in which the coordinates are displayed when printing a xr.DataArray since a few versions ago.

Relevant issues/PRs:

Related to #646

TODO:

  • Write tests for the new fft and ifft functions
  • Remove xrft as a dependency
  • Implement the padding functions.
  • Replace xrft for the new functions throughout the codebase.
  • Make the fft functions public
  • Add tests. Maybe for sake of time, just compare with xrft results.
    • Open issue to add our own tests against analytical solutions.

Add our own private functions to compute the fft and ifft of
a `xr.DataArray`.
Remove other mentions of xrft in docs.
@leouieda

Copy link
Copy Markdown
Member

@santisoler could you make the FFT functions public? I've been using xrft a lot in classes along with Harmonica and it would be useful to have this.

}

# Compute FFT
fft = np.fft.fftshift(np.fft.fftn(grid.values))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@santisoler it would be good to include parts of xrft that calculate the theoretical Fourier Transform instead of just the FFT result. That's in the true_phase and true_amplitude parts of the code: https://github.com/xgcm/xrft/blob/master/xrft/xrft.py#L462

For amplitude based filters this doesn't matter too much since it's all relative. But for spectrum analysis and other possible filters it could matter. So maybe we should add it always to be safe.

@santisoler
santisoler marked this pull request as ready for review August 27, 2026 22:26
@santisoler

Copy link
Copy Markdown
Member Author

I think this is quite ready for our immediate goals of removing xrft as a dependency.

In the process, I've been thinking about improving how non-dimensional coordinates are handled. I don't quite like the fact that users need to drop coordinates before passing the xr.DataArray to the FFT or padding functions.

Maybe we should assume that the padding will only get applied to dimensional coordinates (coordinates that have the same name as the dimension, i.e. they are Index), and keep the other coordinates padded with nans (as the default behaviour in xarray.DataArray.pad. The unpad function could remove additional elements from all coordinates, so a unpad(pad(grid)) will be truly a roundtrip even if the grid contains non-dimensional coordinates.

Similar thing with FFT: we could only compute frequencies using the dimensional coordinates and ignore the other ones when building the grid in the frequency domain.

I'll open an issue for this after merging this PR.

@santisoler santisoler added this to the v0.8.0 milestone Aug 27, 2026
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.

2 participants