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

1999: Clip fitting values set outside the fit range #2422

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Jan 18, 2023

Description

As described in bumps/bumps#58 and #1999, when the initial fit values are set outside the fitting bounds, the MPFit (L-M) and Quasi-Newton fitters fail to converge and may throw errors. This utilizes the bumps FitDriver.clip() feature to coerce the initial values within the bounds prior to fitting. A warning is logged when any values are coerced.

Fixes #1999

This fix should also be added to v5.0.6 once it's reviewed.

How Has This Been Tested?

Run SasView, select the L-M (MPFit) optimizer, load a data set, send it to fitting, and select a model. From there, I tried 2 different things

  1. Set the fit range of one of the parameters to not include the initial value. This was done by changing both the upper a lower limits.
  2. Set the initial value of one of the parameters so it is outside the initial bounds of the fit.

Select the modified fit parameter to be fit and run the fit.

Previously, the fit would fail with an assertion error. Now the fit succeeds and a warning message is given.

I did not test the Quasi-Newton optimizer.

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@krzywon krzywon requested a review from smk78 January 18, 2023 16:08
Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Functionality test on W10/x64 . Local build. Tested with the new MPfit L-M, N-M and Q-N optimisers.
This fixes the issue I reported in reopened #1999 . The error message no longer appears and Min, Max or Min & Max bounds are respected.
If the Min bound is outside the expected fitting range the message in BumpsFitting line 394 is displayed.
Approved.

@wpotrzebowski wpotrzebowski merged commit 2b0b11f into main Jan 24, 2023
@wpotrzebowski wpotrzebowski deleted the 1999-clip-parameters branch January 24, 2023 15:30
@wpotrzebowski
Copy link
Contributor

I reviewed the code and merged it. It can be now cherry picked for 5.0.6.

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.

5.x: Parameter limits in the Fit Page are not respected.
3 participants