modify the truthiness check

[rozyczko]

Description

In the param_remap_to_sasmodels_convert method, the condition if not value: incorrectly treated the value 0.0 as false. When orientation parameters like theta or phi were set to 0, they were being replaced with np.nan, breaking subsequent fits.

Fixes #3053

How Has This Been Tested?

Local tests

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

• There is nothing that needs documenting
• Documentation changes are in this PR
• There is an issue open for the documentation (link?)

Installers

• There is a chance this will affect the installers, if so
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

• The introduced changes comply with SasView license (BSD 3-Clause)

[DrPaulSharp]

Very good spot here @rozyczko!

A couple of things from me, firstly, does this fix also need to be applied to the following lines dealing with the uncertainty and min/max values? It looks to me like they could trip on a zero value as well. Secondly, do you have an example of a bug that this branch fixes? I tried the instructions in #3053 but couldn't reproduce the bug there.

[krzywon]

This is a needed change, but I'm not seeing any improvement in the magnetic/2D fitting functionality. I think this should be merged, but don't think it fixes the underlying issue in #3053. I'll update the issue on recreating the problem later today.

[rozyczko]

The magnetic parameter values stored in MagnetismWidget.magnet_params were not being applied to the kernel_module before fitting. When only magnetic parameters were selected for fitting, the fitter used default sasmodels values instead of user-specified values. Including non-magnetic parameters worked because those triggered other code paths that updated the model correctly.

Added updateKernelModelWithExtraParams(model) call before fitting to ensure magnetic parameter values are synced to the kernel_module.

Added immediate sync to kernel_module when magnetic parameters are initialized.

Also, fixed typos in the polydisp widget.

[krzywon]

This seems to be working now! I get real errors and the magnetic params change as a part of the fit. Code looks good.

[krzywon]

@DrPaulSharp, when you are able, could you compare the behavior outlined in #3053 comment in both this branch and in main? I want to be sure others are able to see the difference in behavior before merging.

[DrPaulSharp]

Hi @krzywon I've tried this, and on this branch I do find that the values change on a fit, but the errors are still extremely large:

[dehoni]

I've followed the steps in the issue 3053. For creating theory curves it shows correct results without getting stuck at angles set to zero for both orientation and magnetism.

The confusing difference is that the model and magnetism tab have different behaviour: for orientation, the scattering cross section is recomputed once the values have been changed and confirmed by hitting "Enter" or pressing "Compute/Plot".
In the magnetism tab, pressing "Enter" after changing values has no effect and you will have to "compute/plot" to recalculate.

[dehoni]

Fitting a cylinder, which is 5 degrees off from 0 in theta and phi works and gives the expected results.

[dehoni]

The fit converges also for magnetic angles, without large error bars when considering only magnetic scattering

Taking also nuclear scattering into the fit, the error can get out of bounds like @DrPaulSharp has seen. In particular up_theta has a tendency for very large errors. This is because setting all theta angles to zero will produce a circular isotropic scattering pattern, which looks like a nuclear isotropic scattering. The angle theta weights the ratio of nuclear against magnetic scattering to the isotropic pattern, which makes the absolute value not well defined.
For most cases the angles are defined and fixed by the scattering geometry.

[krzywon]

@dehoni - For the changes made in this PR, are they meaningful enough to push getting them into v6.1.3, or should we correct the issues you've noted before merging? Whichever way you see this, please give an approval or disapproval.

As far as I can tell, the only functional issue noted is, when hitting Enter in the model parameters window a recalculation is not triggered, but it does in the magnetism widget. There are other usability issues, but those require pre-knowledge on the system being studied, specifically on the different field angles and their orientation to the neutron beam, to ensure the calculations run properly. Does this summarize your findings?

[dehoni]

Correct. It is easy to get coupled parameters, which can produce unrealistic uncertainties. So it is on the experimenter to check it carefully.
I'll open a separate issue to look into the different response behaviour for the tab.

[krzywon]

Post merge note. This modifies FittingController.py which is not in the 6.1.3 release branch. This fix will not be included in v6.1.3.