-
Notifications
You must be signed in to change notification settings - Fork 32
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
Harmonic analysis #613
Harmonic analysis #613
Conversation
I don;t understand why tests are failing |
I started this in the hope of improving the resolution after the discussion in #556 but the windowing did not work very well for the interpolated FFT. We may want to add the NAFF algorithm as a 3rd method in the future. |
Looking only at the first harmonic for get_tune (which is what should be done) solves the test issue |
In fact using 2 harmonics is safer for real input tbt input data |
There is a C implementation of NAFF already in the repository, it was used in Matlab, though apparently it's not proposed anymore. It should be fast enough, however it requires full testing. Apart from that, is this one ready and tested ? |
I have tested it and believe it is ready, but it would be good if @oscarxblanco, @lcarver and @carmignani. I particular @carmignani complained about the lost information on the phase form the previous version so I would like to know whether this is satisfactory, if possible I would not want to come back to it |
Wasn't there a license issue with the NAFF version present in the direction? I can't remember. If not we can use it in fact |
@swhite2401 the licence was GPL-3.0 and should be ok. |
It seems you removed the parameter argument num_harmonics from get_tunes_harmonic. File "/home/sources/physmach/blanco-garcia/codes/pyenv/venv_test_harmonic/lib/python3.10/site-packages/at/physics/frequency_maps.py", line 230, in fmap_parallel_track
xfreqfirst = get_tunes_harmonic(xfirst, num_harmonics=1)
TypeError: get_tunes_harmonic() got an unexpected keyword argument 'num_harmonics' Is there a replacement ? |
@lnadolski ensured that there was no licensing problem with this implementation. @oscarxblanco: |
Yes I did, since the tune is supposed to be the main harmonic I thought this argument was useless. I case many lines are needed one should use the function get_spectrum. If you think this is an issue it is very easy to restore and set the default to 1 |
It is true it also breaks backward compatibility... I will restore it with a default to 1 it is safer |
I removed the argument num_harmonics=1 in my local copy of frequency_maps.py and the calculation of a frequency map finishes without errors. The resulting diffusion plot appears to be the same wrt to a previous calculation (I attach an image). With respect to precision, I tested get_tunes_harmonic with a sin function and tried to recover the frequency while varying the number of samples (no noise added, just numerical precision). I attach a figure, but, in general I see a similar of better result for larger number of samples with respect to what was discussed in #556 . I think one could safely remove num_harmonics=1 from lines 230, 235, 240 and 245 in the file frequency_analysis.py if such modification is included in this PR. However, it might be still a good idea to keep the backwards compatibility. It might be possible that num_harmonics is used in some other part of the code or related codes. A warning could be shown in case it is decided to remove it on another release. |
Thanks @oscarxblanco , I have made the requested changes and restored backward compatibility |
One thing is odd, though. The phase calculated with standard FFT is wrong and I can't figure out why. Any idea? |
I did a test and it works as expected. |
Could you provide an example showing inputs, outputs and call to the function ? |
Here is the code: import at
import numpy
from at.physics import get_main_harmonic
x = numpy.arange(1024)
qx = 0.23
y = numpy.cos(2*numpy.pi*qx*x+numpy.pi)
tune1, a1, p1 = get_main_harmonic(y, method='fft')
tune2, a2, p2 = get_main_harmonic(y, method='interp_fft')
print('')
print(tune1, tune2)
print(a1, a2)
print(p1, p2) and the output
|
While the amplitude can be different I was expecting a phase of ~pi in both case. Increasing the number does not bring the result to the correct value. |
@swhite2401 It seems to be just an offset on the phase. Are you using functions defined on the same interval ? |
Ah thanks @oscarxblanco! In fact this was just stupid... taking the periodic signal everything is fine. Hanning window is making the signal periodic but also shifts the phase. See result below for 2000 turns: All is ok, and in any case on offset is not really detrimental as you generally look at phase differences. So this is ready to merge for me, any other comments? |
I still get a difference in phase. Is there anyway to avoid using hann ? It seems to be false by default ...
|
Ah this is strange... hann is turned off by default. Did you change the number of turn to 2000? To get the correct phase you need to have nturns*qx=integer |
Good! Do you agree to merge then? Can you approve the PR or was there anything else than needs to be changed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_tunes_harmonic function was tested in a particular case of a frequency map, and works as expected.
get_main_harmonic with method fft and interp_fft were tested.
With fft the returned phase behaved differently with the number of points. This seems to be a minor issue considering that the function returns the correct frequency value.
Perfect I merge! |
The
harmonic_analysis
module is rewritten and cleaned up. I have done test to evaluate the effect windowing and padding for the 2 methods and came to the conclusion that they are not useful for the interpolated FFT case.A warning is now issue when users try to activate windowing or padding using interpolated fft.
The method name is now
interp_fft
to be more consistent with what it really does, the old keywordlaskar
is kept for backward compatibility.Returning only the tune was found to be inappropriate for tbt analysis, the amplitudes and phases are now returned in addition to the tune in an additional function
get_main_harmonic
and the existingget_spectrum_harmonic
.No change to other internal AT function, except for the help, only
get_spectrum_harmonic
is not backward compatible.