Skip to content

Allow user to specify output domain (fourier) for pipeline - #29

Merged
paulmueller merged 24 commits into
mainfrom
28_fourier_domain_pipeline
Jul 6, 2026
Merged

Allow user to specify output domain (fourier) for pipeline#29
paulmueller merged 24 commits into
mainfrom
28_fourier_domain_pipeline

Conversation

@PinkShnack

@PinkShnack PinkShnack commented Jun 8, 2026

Copy link
Copy Markdown
Member

As described in #28, I want to allow the user to output the fourier-domain field directly without doing the final iFFT. This removes an unnecessary Fourier transform. When combined with nrefocus (branch-link here) this will remove two unnecessary Fourier transforms and is ~30% faster than the original "spatial" pipeline. The total pipeline (with nrefocus is about 20% faster #29 (comment)).

I went away from doing skip_ifft as discussed in #28 because input/output_domain='spatial/fourier' is broader and works in nrefocus nicely. I also thought this would be much simpler, but having the ability to keep the metadata with the outputted Fourier-domain field is important for the final iFFT.
The final iFFT can be done with either qpretrieve or nrefocus (which calls the qpretrieve object in the background).

Ignoring propagation with nrefocus, the legacy (old) and direct (here, new) pipelines are always the same. When dealing with nrefocus, depending on the user's choice of qpretrieve input params, the old and new pipelines may or may not differ slightly. See the new example in the docs showing how padding effects the final output.

Todo

  • update changelog
  • allow user to set output_domain
  • keep metadata with fft-field (done in the artifact class)
  • easy user method for computing final ifft (oah_obj.compute_field).
    • artifact.finalize is the internal equivalent
  • write tests for qpretrieve specifically
  • benchmark qpretrieve
  • benchmark qpr + nrf (branch link done in tests/req.txt)
    • move to bar nrefocus dependency in req file
  • run this branch on the matching nrefocus syntax in (link branch)
  • set the requirements for tests and examples to the correct nrefocus versions
  • add added version 0.6.x everywhere necessary
  • add docs section describing this new feature
  • is this a feature or enh?
  • inspect the peak finding issue (tracked in bug: peak-finding for sideband not exact for unpadded example data #30)

@PinkShnack

Copy link
Copy Markdown
Member Author

Benchmark of the qpretrieve-only pipeline:

  • fourier does not include the final ifft (which is why it is fastest)
  • fourier_then_spatial is the old pipeline broken into two steps
  • spatial is the old pipeline
tests\test_bm\test_bm_oah_output_domain.py ...                                                                                                                                                                          [100%]


---------------------------------------------------------------------------------------------------------- benchmark: 3 tests ----------------------------------------------------------------------------------------------------------
Name (time in us)                                             Min                   Max                  Mean             StdDev                Median                 IQR            Outliers         OPS            Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_bm_oah_output_domain_fourier[64]                    576.6000 (1.0)      1,628.1000 (1.02)       639.0052 (1.0)      71.1883 (1.17)       625.7000 (1.0)       39.3750 (1.0)         93;99  1,564.9326 (1.0)        1145           1
test_bm_oah_output_domain_fourier_then_spatial[64]       882.6000 (1.53)     1,900.6000 (1.19)       954.7940 (1.49)     60.6353 (1.0)        935.2000 (1.49)      43.2000 (1.10)       108;71  1,047.3464 (0.67)        828           1
test_bm_oah_output_domain_spatial[64]                  1,028.7000 (1.78)     1,599.9000 (1.0)      1,148.6064 (1.80)     94.5220 (1.56)     1,151.4000 (1.84)     124.9000 (3.17)       159;11    870.6202 (0.56)        466           1
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

@PinkShnack

Copy link
Copy Markdown
Member Author

Benchmark of the qpretrieve+nrefocus pipeline (nrefocus branch is currently local).

  • Fourier pipeline is ~20% faster than the legacy spatial pipeline.
tests\test_bm\test_bm_full_pipeline.py ...                                                                                                                                                                              [100%]


------------------------------------------------------------------------------------------------------ benchmark: 3 tests ------------------------------------------------------------------------------------------------------                                                                                                                                                                                                                              
Name (time in us)                                      Min                   Max                Mean              StdDev              Median                 IQR            Outliers  OPS (Kops/s)            Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------                                                                                                                                                                                                                              
test_bm_full_pipeline_fourier_no_finalize[64]     607.2000 (1.0)      4,476.8000 (2.66)     653.8315 (1.0)      105.4580 (1.16)     645.5000 (1.0)       19.4500 (1.0)         28;74        1.5294 (1.0)        1464           1
test_bm_full_pipeline_fourier[64]                 739.0000 (1.22)     3,365.5000 (2.00)     878.5532 (1.34)     152.7670 (1.68)     814.8000 (1.26)     170.9000 (8.79)       133;40        1.1382 (0.74)       1206           1
test_bm_full_pipeline_spatial[64]                 859.8000 (1.42)     1,682.1000 (1.0)      927.0214 (1.42)      91.1290 (1.0)      904.7000 (1.40)      26.6000 (1.37)        22;41        1.0787 (0.71)        322           1
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------            

@PinkShnack

Copy link
Copy Markdown
Member Author

Some points and questions @paulmueller. I would like you input if you have time.

  • Should this be an enh or a feat? I have it as an enh right now.
  • I used input/output_domain as the param name here and in nrefocus for consistency.
  • I named the Fourier-domain output object as FourierFieldData. WHat do you think of this, is it clear? The user shouldn't really ever have to think about this, so the question is more of understanding for us.
  • In the docs example I have the qpretrieve + nrefocus pipeline. Have a look at the code and plot output if you have time. At first glance it is strange, as changing the padding completely changes the output, but this is rather expected and makes sense to me. What other way would there be to show that the first column "matches" and the second doesn't?

@PinkShnack
PinkShnack marked this pull request as ready for review June 18, 2026 09:39
@PinkShnack
PinkShnack requested a review from paulmueller June 18, 2026 09:39
@paulmueller

paulmueller commented Jun 19, 2026

Copy link
Copy Markdown
Member

Some points and questions @paulmueller. I would like you input if you have time.

* Should this be an `enh` or a `feat`? I have it as an `enh` right now.

I would say feat. Bump the minor version.

* I used `input/output_domain` as the param name here and in nrefocus for consistency.

Makes sense.

* I named the Fourier-domain output object as `FourierFieldData`. WHat do you think of this, is it clear? The user shouldn't really ever have to think about this, so the question is more of understanding for us.

I think this is good.

* In the docs example I have the qpretrieve + nrefocus pipeline. Have a look at the code and plot output if you have time. At first glance it is strange, as changing the padding completely changes the output, but this is rather expected and makes sense to me. What other way would there be to show that the first column "matches" and the second doesn't?

The linear tilt in the phase (left column) when padding is disabled might confuse the user. Do you know where this comes from? A tilt in the phase hints toward an incorrect frequency of the sideband detected for the hologram.
What do you want to show with the example?
When you don't pad, then the difference between "fourier-domain" and "spatial-domain" pipelines is that you are performing two additional Fourier transforms (to and from Fourier space). You only see floating-point accuracy errors here.
When you pad, then the same argument is true (I would expect the same error). Where does the difference come from? In the plot (lower right), it looks like it is only at the boundary of the image. What happens if you set padding to 2? Note that the padding kwarg for nrefocus.refocus does something different. So this might just be a confusion of how parameters work due to inconsistencies across the libraries.

@PinkShnack

PinkShnack commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

The linear tilt in the phase (left column) when padding is disabled might confuse the user. Do you know where this comes from? A tilt in the phase hints toward an incorrect frequency of the sideband detected for the hologram.

The sideband is found correctly by qpretrieve (red dot in top row). Here is a plot with a square input (to allow padding=0), and with padding=1,2 also. The tilt occurs regardless of filter size. I guess the lack of padding combined with the off-axis acquisition results in this tilt. Thoughts?

filtered_fft_sidebands

Python script:
filtered_fft_sidebands.py

@PinkShnack

Copy link
Copy Markdown
Member Author

When you don't pad, then the difference between "fourier-domain" and "spatial-domain" pipelines is that you are performing two additional Fourier transforms (to and from Fourier space). You only see floating-point accuracy errors here.
When you pad, then the same argument is true (I would expect the same error). Where does the difference come from? In the plot (lower right), it looks like it is only at the boundary of the image. What happens if you set padding to 2? Note that the padding kwarg for nrefocus.refocus does something different. So this might just be a confusion of how parameters work due to inconsistencies across the libraries.

Comparing the spatial and fourier pipelines you get (with scale_to_filter=False):

  • padding=0: spatial − fourier = 2.952e-14
  • padding=1: spatial − fourier = 1.852 (expected)
  • padding=1, central crop: spatial − fourier = 0.02885
  • padding=2: spatial − fourier = 1.817 (expected)
  • padding=2, central crop: spatial − fourier = 0.02872

The reason (afaik) that the paddded pipelines are different is that:

  • qpretrieve outputs an iFFT that has been cropped (unpadded). This is then padded by nrefocus and FFT'd before propagation. The nrefocus padded FFT and the qpretrieve padded FFT are not identical.
  • I had (also) assumed that the pipelines would be identical but apparently not. However, most of the differences ARE in the border region.

If you set scale_to_filter=True then of course the pipelines will differ.

What do you want to show with the example?

That the padded comparison bewteen spatial and fourier pipelines are not (as I expected) identical.

@paulmueller

Copy link
Copy Markdown
Member
image

This is definitely a peak-finding problem or something similar. Some instability in qpretrieve.

@PinkShnack

PinkShnack commented Jul 1, 2026

Copy link
Copy Markdown
Member Author
image This is definitely a peak-finding problem or something similar. Some instability in qpretrieve.

I tried some alternative peak finding algorithms and they all appear to have issues with this sample. I think we can create an issue to track this. Right now, I would remove the example and instead clarify in the docs that the unpadded pipeline remains identical between spatial and fourier domains, whereas the padded version doesn't (esp. around the image edges, due to padding).

Tracked in #30

@PinkShnack

Copy link
Copy Markdown
Member Author

Hey @paulmueller I am tracking the peak finding issue in #30. I made the above changes to the examples and docs. If sufficient please let me know and we can merge and release :)

@paulmueller
paulmueller merged commit 84b334f into main Jul 6, 2026
24 checks passed
@PinkShnack
PinkShnack deleted the 28_fourier_domain_pipeline branch July 7, 2026 09:33
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