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

Move from 2d to 3d array operations #12

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

Conversation

PinkShnack
Copy link

@PinkShnack PinkShnack commented Jan 7, 2025

About

We should allow stacks of 2D arrays as inputs. This will likely speed up processing of large datasets, and certainly will when we add Cupy as an FFTFilter (#10). Several things to note:

  • Allowed formats e.g. RGB, 2d, 3d (see below)
  • What should be returned to the user: currently, a 3d array with the shape (z, y, x), with z being the number of images in the stack.
    • We can provide convenience functions for users to convert between formats. I think we should limit this as a "per image" basis. In other words, the functions should handle image input, not qpretrieve classes. We could f course have some nice methods that do the conversion within the class, and return the image to the user.
    • Important to have a method that returns the original and current image formats.

To do

  • FFTFilter Base class work with 3D arrays
  • QLSI init - fix the instanciation - allow user to set FFTFilter
  • run_pipeline steps for OAH and QLSI should also work with 3D arrays.
  • Benchmark single 2d array prcessed n times against stack of n images all processed at once. Put in docs. See test test_fft_comparison_data_input_fmt
  • Image formats. The following formats should continue to work.
    • 2D images (y, x) should be converted internally to (z, y, x)
    • 3D images (image stacks) (z, y, x) will work with above
    • 2D RGB images (y, x, 3) or RGBA (y, x, 4). The data will be assumed to be greyscale, and each channel equal. The first channel will be taken and converted as in the 2D images case above.
      • A warning should be passed to the user.
    • The shape of the returned field will be 3D, and we will provide convenience functions for conversion back to the original format.
      • tests for each data format and each data_attr
  • docs
    • compare old (3d input -> 3d output) and new versions (2d input -> 3d output)
    • sphinx not collecting get_array_with_input_layout in code reference
    • show image format conversion
    • base class data argument should be documented clearly
    • show benchmark for 2d vs 3d.
    • Clearly describe what data shapes are handled internally, and what is not (yet) handled e.g. stack of rgb: (20, 128, 128, 3).
    • add type hints
  • tests:
    • data array layouts from 3d (test_data_array_layout_to_3d.py)
    • hologram fixture not inputting size argument Solution was to use "request" as hologram size param for the fixture. This is required as learned here.
      • Make PR on pytest describing this
    • compare 2d and 3d FFT algorithms, ensure consistency
  • cicd passed
  • update changelog

@PinkShnack PinkShnack added the enhancement New feature or request label Jan 7, 2025
@PinkShnack PinkShnack changed the title 9 feat br 3d arrays Move from 2d to 3d array operations Jan 14, 2025
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@PinkShnack PinkShnack mentioned this pull request Jan 21, 2025
@PinkShnack PinkShnack changed the base branch from main to ci_and_doc_fixes January 21, 2025 12:55
@PinkShnack PinkShnack changed the base branch from ci_and_doc_fixes to main January 21, 2025 12:55
@PinkShnack PinkShnack changed the base branch from main to cupy_gpu_fft_backend January 21, 2025 12:56
@paulmueller paulmueller changed the base branch from cupy_gpu_fft_backend to main January 21, 2025 12:56
@PinkShnack
Copy link
Author

Hey @paulmueller, when you get a chance please go through this PR. I added docs which you should look through. Main things left are "QLSI init - fix the instanciation" and updating the changelog.

Copy link
Member

@paulmueller paulmueller left a comment

Choose a reason for hiding this comment

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

I only have a few comments. It's not yet waterproof, but it's almost there.
Thanks!

data_3d_prep, _ = convert_data_to_3d_array_layout(data_2d)
data_3d_bg_prep, _ = convert_data_to_3d_array_layout(data_2d_bg)

for fft_interface in fft_interfaces:
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is that you are not taking into account FFTW wisdom (https://en.wikipedia.org/wiki/FFTW). FFTW should be much faster than numpy, because it initially tests several FFTs on the input data shape and then takes the fastest one. The wisdom is forgotten unless you store it locally on disk everytime you use pyfftw (#5).

I assume that if you added PyFFTW a second time to the fft_interfaces list, you will get faster results (because the wisdom is already there from the first run).

Copy link
Author

Choose a reason for hiding this comment

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

I can add a third one without the initial wisdom

Copy link
Author

Choose a reason for hiding this comment

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

FYI, the wisdom only is a factor in the initial batch 8 run:

image

Copy link
Member

Choose a reason for hiding this comment

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

Something is not right here. The FFTW with wisdom must always be faster than the one without wisdom. Did you measure the time with time.perf_counter()?

Copy link
Member

Choose a reason for hiding this comment

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

It could also be that we are not using PyFFTW correctly in qpretrieve. I always thought wisdom is handled automatically.

Copy link
Author

Choose a reason for hiding this comment

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

Isn't what we are seeing here that the wisdom is calculated during the first loop (batchsize 8), and thereafter Pyfftw uses this for all further calculations?

Copy link
Member

Choose a reason for hiding this comment

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

PyFFTW should compute the wisdom for every batch size individually. For batch sizes 8 and 256 the one with wisdom is slower than the one without wisdom. I would not have expected that.

Copy link
Member

Choose a reason for hiding this comment

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

I think you did not use time.perf_counter in your script. Maybe you can try that. It could explain this, since you are normalizing by batch size.

Copy link
Author

Choose a reason for hiding this comment

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

fft_batch_speeds

Here I have compared pyfftw (not including wisdom calulation time) with numpy. Should be resolved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants