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

ksw/enhance image fitting performance by switching the solver from qr to cholesky #1114

Merged
merged 1 commit into from
May 27, 2022

Conversation

kswang1029
Copy link
Contributor

@kswang1029 kswang1029 commented May 26, 2022

This is to address the performance concern from @Jordatious about the current 2D image fitting implementation by switching the default solver "qr" to "cholesky". You may refer to
https://www.gnu.org/software/gsl/doc/html/nls.html#c.gsl_multilarge_nlinear_solver.gsl_multifit_nlinear_solver_qr
https://www.gnu.org/software/gsl/doc/html/nls.html#c.gsl_multilarge_nlinear_solver.gsl_multifit_nlinear_solver_cholesky
to see the differences as also mentioned in the comment chain of the PR #1055.

With some tests, the performance boost in terms of elapsed time is reduced by ~66%. This is measured by testing single and triple gaussian fit with different image sizes. As for the fitting result accuracy, it appears to me that the two solvers give very similar solutions.

I also tried other methods to solve the trust region subproblem (https://www.gnu.org/software/gsl/doc/html/nls.html#solving-the-trust-region-subproblem-trs)
We use gsl_multifit_nlinear_trs_lm : Levenberg-Marquardt algorithm now:
gsl_multifit_nlinear_trs_lmaccel : Levenberg-Marquardt algorithm with geodesic acceleration --> slower. I don't provide the second directional derivative vector so it is using finite difference method for approximation
gsl_multifit_nlinear_trs_dogleg: dogleg algorithm --> slower
gsl_multifit_nlinear_trs_ddogleg: double dogleg algorithm --> slower
gsl_multifit_nlinear_trs_subspace2D: 2D subspace algorithm --> super fast and converged with just 1 iteration but the result is not correct. Not sure if I miss something configurable here

This PR is considered as a quick-and-dirty improvement of the fitting performance for the final v3 release. Open for comments on whether or not merging this.

@veggiesaurus
Copy link
Collaborator

Looks good. Can you provide some benchmarks with various image sizes?

Copy link
Contributor

@acdo2002 acdo2002 left a comment

Choose a reason for hiding this comment

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

I used Cypress to measure the performance.
The carta-cypress code is in "carta-cypress/cypress/integration/carta/2DGaussianFitting.spec.js" and the testing image (simple 1 component Gaussian image) is "Gaussian2Dimage.fits" in here.

The current backend dev branch:
image
It takes ~21sec to do the 2D image 1 component Gaussian fitting.

While in ksw branch:
image
It only takes 10sec to complete the same fitting.
I think the performance has significant improvement.

@veggiesaurus
Copy link
Collaborator

I used Cypress to measure the performance. The carta-cypress code is in "carta-cypress/cypress/integration/carta/2DGaussianFitting.spec.js" and the testing image (simple 1 component Gaussian image) is "Gaussian2Dimage.fits" in here.

Thanks @acdo2002. Can you run a similar test using the much larger HST image?

@acdo2002
Copy link
Contributor

acdo2002 commented May 27, 2022

@veggiesaurus Yes, here is the result. I used the Abell 1689 HST public archive WFC3 image "ibja08020_drc.fits" (4131 x 4388 pix, 218MB), which is much larger than "Gaussian2Dimage.fits" (1000 x 1000 pix, 8MB)

The current dev branch:
image
It takes 180 sec (~3min) to derive a single component 2D gaussian fitting (fit the foreground star)

While in this ksw branch:
image
It only takes 67sec (~1min) to derive the same fitting result.

@veggiesaurus veggiesaurus merged commit 6efe4ad into dev May 27, 2022
@veggiesaurus veggiesaurus deleted the ksw/gsl_fit_tuning branch May 27, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants