Skip to content

Add ONNX benchmark #381

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

Merged
merged 20 commits into from
Apr 3, 2025
Merged

Add ONNX benchmark #381

merged 20 commits into from
Apr 3, 2025

Conversation

tkoskela
Copy link
Contributor

No description provided.

@tkoskela tkoskela marked this pull request as ready for review February 24, 2025 16:29
Comment on lines +216 to +219
m_fb = factory::fb_factory<sopt::algorithm::ImagingForwardBackward<t_complex>>(
factory::algo_distribution::mpi_serial, m_measurements_distribute_grid, wavelets, m_uv_data,
m_sigma, beta, gamma, m_imsizey, m_imsizex, m_sara.size(), state.range(3), true, true, false,
1e-3, 1e-2, 50, tf_model_path, nondiff_func_type::Denoiser);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the measurement operator normalisation calculated anywhere? It doesn't appear to be in AlgoFixtureMPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No? I'd assume it just uses the default 1.0

Copy link
Contributor

@mmcleod89 mmcleod89 left a comment

Choose a reason for hiding this comment

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

I think it's fine as long as we can confirm that the measurement operator is being normalised properly (if that's necessary for this benchmark), and if we don't need the code in the if(false) statement then we should probably just delete it.

@tkoskela
Copy link
Contributor Author

tkoskela commented Mar 20, 2025

Something is broken with homebrew on the MacOS runners 😞

Today it works 🥳

Copy link

@kmulderdas kmulderdas left a comment

Choose a reason for hiding this comment

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

Other than a parameter change in one of the benchmarks, I don't see any problems

Copy link

@kmulderdas kmulderdas left a comment

Choose a reason for hiding this comment

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

Addresses the changes I requested.

@tkoskela
Copy link
Contributor Author

tkoskela commented Apr 2, 2025

Ubuntu CI is failing due to actions/runner-images#11926

@tkoskela tkoskela merged commit 19bf277 into development Apr 3, 2025
16 checks passed
@tkoskela tkoskela deleted the tk/onnx-benchmark branch April 3, 2025 09:16
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.

3 participants