mantidproject / mantid

Main repository for Mantid code
https://www.mantidproject.org
GNU General Public License v3.0
210 stars 122 forks source link

Fitting parameters not being constrained #36906

Closed GuiMacielPereira closed 1 month ago

GuiMacielPereira commented 6 months ago

Describe the bug

Sometimes the constraints on fitting parameters are not obeyed by the fit. Found during smoke testing. Doesn't happen in all functions or parameters, but happens for the case detailed below:

To Reproduce

  1. Load EMU00020884
  2. Click on Plot > Spectrum > plot spectrum number 6
  3. Click Fit
  4. Left Click on plot > Add other function
  5. Click Fit > Fit
  6. Then change the constraints on the Lifetime parameter:
  7. image

  8. Perform Fit again
  9. Fit did not obey constraints:
  10. image

Expected behavior Parameter sits within the range.

Platform/Version (please complete the following information):

Additional context I also noticed that if for example you try to do constraints on the Height Parameter, it usually works and makes the constraints on Lifetime parameter be obeyed as well.

sf1919 commented 6 months ago

Does this also occur in v6.8?

GuiMacielPereira commented 6 months ago

Yes, I forgot to say, this was also present in 6.8

GuiMacielPereira commented 5 months ago

After some investigating, here is what I think is happening:

The constraints are implemented through a penalty function at least in several of the minimizers (DampedGaussNewton, Deriv, Fabada, LevenbergMarquardtMD, Simplex, TrustRegion). The penalty is calculated by using a penalty factor and the distance to the boundary limit (see documentation https://docs.mantidproject.org/nightly/concepts/FitConstraint.html).

This means that the parameter size is relevant: for very big parameters, the distance to the boundary will be big and so the penalty function that gets added to the cost function will take up most of the cost function value. This means that during the minimisation procedure, the minimizer will prioritise enforcing the constraint rather than producing a good fit. On the other hand, if the scale of the parameter is small, the opposite will happen: the impact of the penalty function on the total cost function is negligible, and so the constraints are likely to be ignored.

This particular case of an exponential decay function shows this well: the Height parameter is big in scale and any constraints on it are usually enforced. the Lifetime parameter is small, and so its constraints are usually ignored. The most worryingly aspect is that when there are constraints on huge parameters, like the Height of an exponential decay function, the fit may give totally wrong results:

image

In the example above the lifetime parameter was set to a ridiculous value and the fit still shows as successful.

GuiMacielPereira commented 5 months ago

Another thing I noticed is that in the code the penalty function is calculated as penalty = m_penaltyFactor * dp * dp ( see https://github.com/mantidproject/mantid/blob/main/Framework/CurveFitting/src/Constraints/BoundaryConstraint.cpp#L181) but in the documentation the distance is not squared, so it's just penalty = m_penaltyFactor * dp https://docs.mantidproject.org/nightly/concepts/FitConstraint.html . I am not sure which one is intended.

GuiMacielPereira commented 5 months ago

The final thing I tried was to set a different penalty factor before the minimisation procedure depending on the size of the parameter.

The default penalty factor is 1000, so I tried:

My idea was to try to automatically set a penalty factor that would be small when the parameters are big and would be big when the parameters are small, but unfortunately in both cases I couldn't get the balancing right and the penalty value of one parameter ended up overshadowing the other.

jhaigh0 commented 4 months ago

@GuiMacielPereira Does this issue need transforming into something else if it is working as designed?

GuiMacielPereira commented 4 months ago

I would say to leave it as it is. Changing the optimisation algorithm to treat constraints on the parameters differently is likely an epic in itself, and I don't think it's warranted, as we do have several minimizer options that do enforce the bounds on the parameters differently. So if the users want the bounds to be strictly observed or the fit not to be entirely warped by the bounds constraints, they can use another minimizer that does not use a penalty function.

jhaigh0 commented 1 month ago

Is there any action left to be taken with this issue or can it be closed?

GuiMacielPereira commented 1 month ago

It can be closed :)