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

idft #79

Closed
lanougue opened this issue Oct 17, 2019 · 10 comments
Closed

idft #79

lanougue opened this issue Oct 17, 2019 · 10 comments

Comments

@lanougue
Copy link
Contributor

Hi,
I became quite a fan of this xrft library but I am missing the "idft" function. Is it possible to implement it ?
Ideally, it would transform back a 'freq_x' coordinate back to 'x' coordinate and wrap the ifft function.
Thanks

@rabernat
Copy link
Collaborator

Hi @lanougue - thanks for you comments! xrft is simple, but we find it very useful for our needs.

We would love to have the inverse functions implemented. Is this something you'd like to take on yourself? A pull request would be most welcome.

@lanougue
Copy link
Contributor Author

I can try but I'm not used to play with Github (newbie)...

@lanougue
Copy link
Contributor Author

lanougue commented Oct 29, 2019

Hi @rabernat ,
I extended the dft code for the following points:

  1. forward (df) and backward (idft) fft (sharing the same main code)
  2. Take coordinates lag into account to ensure a=idft(dft(a)) (see comment below)
  3. Allow other prefix than 'freq_'
  4. Change back 'freq_x' into 'x' instead of 'freq_freq_x' (same for 'spacing')
  5. idft is working wether or not frequency coordinate of input is fftshifted (shift= False/True).

Important point:
idft (as well as dft) returns centered coordinates (fftfreq style).If coordinates of input A is of fftfreq style, coordinates of idft(dft(A)) are the same as A (in other words: idft(dft(A)) = A for both amplitude and coordinates). If coordinates of A are not of fftfreqstyle (probably the most common case), A and idft(dft(A)) are the same on common coordinates only (expected behaviour from my point of view).

Consequences:
If theses changes are accepted, this point leads to a different dft output than previous version. However, this will only change the phase of the output, all spectra output will be preserved.

Can you help me for my first pull-request ? Correct me:

  1. Clone dft repository
  2. Create a new branch with my modifications
  3. Pull request or ask for merging ?
    Thanks

@roxyboy
Copy link
Member

roxyboy commented Oct 29, 2019

Thank you for working on this @lanougue ! But, it's kind of hard to exactly understand what you have implemented without actually seeing the code. I personally do not think changing the conventions of dft is a good idea... If you could submit your changes as a PR, we could discuss more details about the changes you've made :) To submit a PR, you would first need to:

  1. Fork the xgcm/xrft repository.
  2. Clone your fork.
  3. Create a new branch in your forked repository and make the edits there.
  4. git push your branch to origin.
  5. Submit a PR from your forked repository on Github.

@lanougue
Copy link
Contributor Author

lanougue commented Oct 29, 2019

Thanks @roxyboy. I will try the PR procedure.

I agree with you on the fact that changing dft convention is generally not a good idea. However, it seems to be a necessity as soon as coordinates start entering into account.
From my point of view, absolute phase of dft is pointless if no coordinates are taken into account. Only difference of phase between two dft may matter (only if coordinates are the same for both input and, in that case, my modifications are transparent).
Moreover, people usually use abs(dft(A))**2 for spectrum computation or dft(A)*conj(dft(B)) for cross-spectrum. In all these cases, the proposed modifications should not change the results.

The proposed modifications purpose only serves for consistency for forth and back transforms when absolute phase matters.

I hope it will be more clear when the code will be shared. Working on it ...

@lanougue
Copy link
Contributor Author

lanougue commented Oct 29, 2019

I tried the PR.
Here is a short example to exemplify my point with uncentered coordinates:

import xarray as xr
import numpy as np
import xrft
a = xr.DataArray(np.random.rand(10),dims=('x',), coords={'x':np.arange(-7,3)})
ta = xrft.dft(a, dim=('x',), shift=False)
tta = xrft.idft(ta, dim=('freq_x',))
a.plot()
np.real(tta).plot(linestyle='--')

@navidcy
Copy link
Contributor

navidcy commented Jun 12, 2020

hey @lanougue, xrft.idft would be great. It's something I want to use as well...
did you end up submitting a PR?

@roxyboy, from what I see this was never implemented, no?

@lanougue
Copy link
Contributor Author

Hi @navidcy,
The PR was submitted but did not pass the tests because of profound modification of what the result should be. Look @ #81 discussion.
I was away from this for some time. I will try to get back in soon!

@lanougue
Copy link
Contributor Author

resolved in #129

@roxyboy
Copy link
Member

roxyboy commented Jan 28, 2021

Resolved by PR #129

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

No branches or pull requests

4 participants