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

Simultaneous fitting returning incorrect fit #1914

Closed
Caddy-Jones opened this issue Sep 16, 2021 · 8 comments
Closed

Simultaneous fitting returning incorrect fit #1914

Caddy-Jones opened this issue Sep 16, 2021 · 8 comments
Labels
Blocker Prevents a different issue from being resolved Defect Bug or undesirable behaviour
Milestone

Comments

@Caddy-Jones
Copy link
Contributor

Caddy-Jones commented Sep 16, 2021

When attempting to use the “constrained or simultaneous fit” functionality with the master branch SasView version, the fit isn’t correct.

For example, the attached plot shows the fit the three tutorial files: AOT_Microemulsion-Core_Contrast.xml, AOT_Microemulsion-Shell_Contrast.xml, and AOT_Microemulsion-Drop_Contrast.xml; using SasView on the Master branch. This seems incorrect and is different to the plots obtained using the current released version of SasView, version 5.0.4, and the demonstration plots in the tutorial. Also the scale and background parameters seem to be constrained without being asked to.

shell_contrast_AOT_me_SANS_incorrect
core_contrast_AOT_me_SANS_incorrect
drop_contrast_AOT_me_SANS_incorrect

When attempting to plot two sets of experimental data, plots also attaches, SasView seems to only use the data from the first fit page for fitting. This behaviour is different form the behaviour of the released version of SasView, version 5.0.4. Again all selected fitting parameters, in this case sld_core, sld_shell and sld_solvent, appear to be constrained with the same values between the datasets. This is without any constraints being added.

SAXS_incorrect_plot
SANS_incorrect_plot

@Caddy-Jones Caddy-Jones added the Defect Bug or undesirable behaviour label Sep 16, 2021
@smk78
Copy link
Contributor

smk78 commented Sep 16, 2021

I concur @Caddy-Jones . But I think it's more serious than that; I think fitting is totally broken! You can't even get a single fit to converge. And it's affecting multiple optimisers. I'm making this a blocker for the moment.
But the question is, what could have caused this?

@smk78 smk78 added the Blocker Prevents a different issue from being resolved label Sep 16, 2021
@smk78 smk78 added this to the SasView 5.0.5 milestone Sep 16, 2021
@smk78
Copy link
Contributor

smk78 commented Sep 16, 2021

Does main now require a later version of bumps? I currently have 0.7.11.

@Caddy-Jones
Copy link
Contributor Author

Caddy-Jones commented Sep 16, 2021

Hi @smk78
After some testing, it appears that the issues come from merging pull request #1682, Uncertainties for constrained parameters. More specifically in the changes to the way SasView fits in the BumpsFitting.py file. Currently I’m unsure what is causing the issue, but when I reverted the changes from this pull request, then SasView was plotting and fitting normally

@gonzalezma
Copy link
Contributor

We also observed that the fitting seems to work, in the sense that the returned chi^2 is ok and the parameters as well, but they are not sent back to the appropriate place in the interface, where they appear mixed (e.g. the radius in the background box, etc.), making the plot meaningless. @smk78, can you confirm if you get this as well?

@smk78
Copy link
Contributor

smk78 commented Sep 17, 2021

Yes, I think I see the same @gonzalezma .
image
In this screenshot you can see a rubbish fit with poor residuals and a poor chi^2 optimising just scale and background. Clearly a not very good job has been done of optimising those parameters. But then when you look closely, the parameters in the uncertainty plot and the Log Explorer (I turned on DREAM to show the parameters optimising) are the same, but are the reverse of what is shown in the FitPage.

(Sorry for the delay in checking by the way; I hadn't built anything on this computer since the start of the pandemic!)

@Caddy-Jones
Copy link
Contributor Author

Hi guys, I have made two pull requests to fix this issue. #1915 just gets rid of the code, from pull request #1682, that is causing the bumps fitting to misbehave. #1916 is my attempt to fix the code, so it’s function is still fulfilled (propagating errors for constrained parameters in this case) without causing incorrect fitting.

#1915 is rendered obsolete by #1916, however is still useful as a quick fix while #1916 is being tested.

@smk78 could you maybe test my pull requests, to ensure you are not getting the same results?

@smk78
Copy link
Contributor

smk78 commented Oct 1, 2021

Just tested #1915. Is good.

@smk78
Copy link
Contributor

smk78 commented Oct 4, 2021

Just tested #1916. Also looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker Prevents a different issue from being resolved Defect Bug or undesirable behaviour
Projects
None yet
Development

No branches or pull requests

3 participants