[ENH] OWFFT: Configure Complex FFT manually#809
Conversation
|
Thanks for this @ngergihun . I did not go through in detail yet but it seems that some tests are failing. A few high-level requests:
FYI I'm currently carrying a patched |
|
I am correcting the tests. I made a change in the UI settings update, so it does not put back the HeNe wavelength to the dx edit field all the time, when you change the input. This messes up one test function.
That was my lint workflow. Tried to revert it not to apply for the old part of the code, but unfortunately, some stuff got committed, my bad. I make sure they are reverted.
I'll have a look. I agree that the naming is sometimes misleading. I guess you don't want an asymmetric (different slopes for negative and positive zpd side) apodization for truncated data, since it distorts the ifg. So far the owfft widget always did that. Now, with this PR, unchecking the asymmetric checkbox, it gives you the symmetric but truncated apodization function.
Okay, so should I remove the apodization-related part of this PR? |
Precisely. This would be great!
If it's not too much work, it would be nice to discuss them separately. |
OK, done. I removed the apodization part. All tests seem to run. |
There was a problem hiding this comment.
I think we should take this opportunity to simplify things rather than make them more complex (haha).
self.use_phaseinput_for_complexfft should just be self.complexfft and controls which calculate*FFT function is called.
self.use_polar_FFT should only inform that the data on the input has the interleaved complex structure. (And maybe rename it to clarify that)
I put some changes above, but also at
try:
channel_data, detail = self.data.attributes["Channel Data Type"]
if channel_data == "Polar":
self.use_polar_FFT = True
ADD
self.complexfft = True
self.controls.complexfft.setDisabled(True)
which should end up with the same behaviour as before when "Channel Data Type" = Polar
| out_limit2 = settings.Setting(4000) | ||
| autocommit = settings.Setting(False) | ||
| use_phaseinput_for_complexfft = settings.Setting(False) | ||
| asym_apod = settings.Setting(False) |
There was a problem hiding this comment.
I guess this can be dropped from this PR
| from Orange.widgets.utils.widgetpreview import WidgetPreview | ||
| WidgetPreview(OWFFT).run(load_test_gsf()) | ||
| data = Orange.data.Table("20250412-TGQ1-O2A-ifg-lowdrift.xyz") | ||
| phase = Orange.data.Table("20250412-TGQ1-O2P-ifg-lowdrift.xyz") |
There was a problem hiding this comment.
This data is not available in the repo
There was a problem hiding this comment.
Yes, sorry, this was changed in the commits regarding the apodization and after removing them, it came back. I'll change it.
| else: | ||
| self.controls.phase_corr.setDisabled(False) | ||
| self.controls.phase_res_limit.setDisabled(False) | ||
| self.controls.phase_resolution.setDisabled(False) |
There was a problem hiding this comment.
This results in some strange GUI behaviour where things get disabled but not enabled when use_polar_FFT = True
|
|
||
| cb0 = gui.checkBox( | ||
| self.optionsBox, self, "use_phaseinput_for_complexfft", | ||
| label="Complex FFT with phase input", |
There was a problem hiding this comment.
Just "Complex FFT" (see general comment below)
| def commit(self): | ||
| if self.data is not None: | ||
| if self.use_polar_FFT: | ||
| if self.use_polar_FFT or self.use_phaseinput_for_complexfft: |
There was a problem hiding this comment.
Just if self.complexfft (re general comment)
| self.Error.clear() | ||
| self.Warning.clear() | ||
|
|
||
| if self.use_phaseinput_for_complexfft: |
There was a problem hiding this comment.
Flip this to if self.use_polar_fft -> interleaved
| wavenumbers_domain, X=phases, metas=self.data.metas[1::2] | ||
| ) | ||
|
|
||
| if self.use_phaseinput_for_complexfft: |
There was a problem hiding this comment.
Flip to if self.use_polar_FFT
|
@stuart-cls, sorry for the late reply, I was busy with measurements.
Thanks for the suggestion for the logic. So as far as I understand the complex fft with external phase or the interleaved complex fft both only run with the complex fft enabled. What should happen if you connect an interleaved datatable while the complex fft checkbox is not checked? Should be just raise and error to warn the user? |
|
@ngergihun no problem, it's morning for me :)
As I understand it, the previous code only recognized the interleaved data by the presence of So the behaviour for that data type is the same as before, but we standardize on a single setting to control complex calculation. I tried to show that with this later piece: Another reason I like this separation is when we find a solution to #808 we can just remove this part but the user interface and settings don't have to change. |
|
@stuart-cls thanks for the clarification. I tried to decomplexify ( :D :D :D) the code as you suggested. Also, made sure that the phase correction controls are enabled again. Please check if you see any stupid mistakes. |
|
Another thing that we discussed with @borondics , that the checkbox solution for the data spacing is sometimes very annoying as it is currently. When you (or in some cases the code) set it checked, the number in the input field is overwritten. Thus it is a bit inconvenient. We thought what if we use radiobuttons with 3 options:
Another question regarding this topic. As far as I see, the |
stuart-cls
left a comment
stuart-cls
left a comment
There was a problem hiding this comment.
This looks great!
There's something funny with the auto HeNe but I agree that it's no longer appropriate. Most data contains the actual spacing used, we should prefer that. I'll put something in another PR.
|
Thanks for your answer and help @stuart-cls, I guess after the others' approval, we could merge this. |
|
Hey Guys! I think it is great. Let's merge it. |
3 participants
Hi Everyone!
The complex FFT using complex-valued interferograms is crucial for asymmetric interferometry data such as DFTS or nanoFTIR.
Currently, the OWFFT widget only calculates complex FFT for a specifically defined datatable organization. Namely, if the data is not read by
NeaReaderGSForNeaReaderMultiChannel, there is no option for the user to do a complex FFT using amplitude and phase input data tables separately.I modified the widget to correct the lack of flexibility of the widget so users can apply the processing to any data.
Additionally, there is an option for asymmetric or symmetric apodization, in general. As far as I heard, @stuart-cls might wanted to have something like this.
Modifications:
stored_phaseinput is used as the phase of the interferogramsThe modified widget should be backward compatible since I have not removed any functionality.