SasView / sasview

Code for the SasView application.
BSD 3-Clause "New" or "Revised" License
51 stars 41 forks source link

5.0.5rc2: radius_effective_mode appears broken in P*S fits #2009

Closed smk78 closed 2 years ago

smk78 commented 2 years ago

Testing 5.05rc2 on W10/x64. This issue is a change from 5.0.4.

Fitting a P*S model in 5.0.5rc2 in radius_effective_mode = radius I find that the constrained radius for S is not updated in the FitPage: image

It does update in 5.0.4: image

Worse, setting radius_effective_mode = unconstrained and checking both radii in 5.0.5rc2 only appears to adjust the P radius: image Notice that the chi-squared did not change from the earlier fit either; this suggests that radius_effective_mode is actually broken.

Whereas in 5.0.4 both radii are optimized and the chi-squared changes: image

smk78 commented 2 years ago

Having re-installed 5.0.5rc1, it seems to behave like rc2.

butlerpd commented 2 years ago

Seem comments posted to #1837 and the discussion in #1525. this may be more subtle than it seems

rozyczko commented 2 years ago

Using current release_5.0.5 branch.

I loaded P123_D2O_10_percent.xml and chose sphere/sphere/hayter_msa Left constrained radius_effective and fitted radius and background. Both values seem the same (barred incorrect formatting of radius_effective.

p1

Switched to unconstrained and fitted again. Got two different values with Chi2 also different.

p2

I probably did something silly, maybe used a wrong data file. Can you please share your SDS_2_2_SANS dataset?

2p0_SDS_D2O.zip

RichardHeenan commented 2 years ago

5.0.5rc2 on windows, tried core_shell_sphere time hard_sphere on AOT_drop data, constrained radius_effective seems OK with monodisp or polydisp fit but value displayed in gui freezes as soon as resolution smearing is added. By deliberately freezing the apparent radius_effective at some silly value, then doing a fit with resolution smearing on, the chi squared is good, so I am pretty sure that the underlying S(Q) is good, but radius_effective is not being reported properly.

RichardHeenan commented 2 years ago

Note that Steve's original example has "custom pinhole smear" included. See https://github.com/SasView/sasview/issues/1966 https://github.com/SasView/sasmodels/issues/488

RichardHeenan commented 2 years ago

Using current release_5.0.5 branch.

I loaded P123_D2O_10_percent.xml and chose sphere/sphere/hayter_msa Left constrained radius_effective and fitted radius and background. Both values seem the same (barred incorrect formatting of radius_effective.

p1

Switched to unconstrained and fitted again. Got two different values with Chi2 also different.

p2

I probably did something silly, maybe used a wrong data file. Can you please share your SDS_2_2_SANS dataset?

Just had a look at this data set and S(Q), 5.0.5rc2 Windows, seems to have same issues as other examples, so no further bad behaviour.

butlerpd commented 2 years ago

That's ... reassuring? @RichardHeenan ? As I recall the radius_effective was developed as part of the beta approximation project so it is not totally surprising that both are affected by the smearing. I would be willing to bet that is also the problem with the reported sesans issue as well. So fixing this would clean up a lot of things at once on something that is at the heart of colloidal SAS modeling.

That said, I also suspect this will not be trivial.

Question: Since it seems to do the right thing mathematically, even if very confusing in how it presents to the user, are we willing to ship with this as a "known issue" and make it a blocker going forward? And/or how long are we willing to hold up this release to get this sorted, given it was presumably problematic in 5.0.4?

This is so fundamental I'm not sure what the right answer is...

rozyczko commented 2 years ago

I see where the problem may be coming from. The Intermediate results part of the results coming from bumps does not contain the value of radius_effective when smearing is present. This is what the GUI is querying. When no smearing is applied, I see volume, volume_ratio and radius_effective in the results.

What can be done in the GUI is to manually update the radius_effective value with corresponding radius for this case.

butlerpd commented 2 years ago

LOLOL ... of course. Makes perfect sense now that you say it. It is not a fit parameter (since it is constrained to be the same as radius) ... so nothing to return (I'm assuming that BUMPS does not return the value of any fixed parameter?

smk78 commented 2 years ago

What can be done in the GUI is to manually update the radius_effective value with corresponding radius for this case.

I presume when you say manually @rozyczko, you mean a GUI code hack, right? Not the user manually updating one or other radius values?

RichardHeenan commented 2 years ago

What can be done in the GUI is to manually update the radius_effective value with corresponding radius for this case.

I presume when you say manually @rozyczko, you mean a GUI code hack, right? Not the user manually updating one or other radius values?

But presumably if that is true there is already a "hack" in the gui or the top end of sasmodels for the unsmeared case which does report a constrained radius_effective ? Which should also be putting S_eff(Q) and beta(Q) into the data explorer? Time to look in kernel.py or whatever it is called, and hope it is fairly obvious.

RichardHeenan commented 2 years ago

I added the reverse links, but we need one here, so see also https://github.com/SasView/sasview/issues/1966 https://github.com/SasView/sasmodels/issues/488

rozyczko commented 2 years ago

I presume when you say manually @rozyczko, you mean a GUI code hack, right?

Correct. I should have used programmatically rather than manually.

But presumably if that is true there is already a "hack" in the gui or the top end of sasmodels for the unsmeared case which does report a constrained radius_effective ?

That's what I would expect, since non-smeared results do contain radius_effective.

RichardHeenan commented 2 years ago

In sasmodels/sasmodels directory,
In product.py the _intermediates function sorts out radius_effective, S_eff(q), beta(Q) etc. [ note for interest there is also _intermediates in mixture.py ] In resolution.py the smearing gets set up, but I have not yet worked out where that is called from to actually get done, as that is likely where the problem occurs. Perhaps someone else can find out, as I need to stop for today.

wpotrzebowski commented 2 years ago

I've tried to reproduce this issue using sasmodels from script but went down the rabbit hole trying to understand what sasview does... No conclussions yet but I am looking if we didn't miss anything with https://github.com/SasView/sasview/commit/8d76d497ea00393c0262b35af5becb3ae509e9dd that changes interface to bumps fitting.

wpotrzebowski commented 2 years ago

Hmm, This becoming even more puzzling. I seem to have same behavior in 5.0.5rc2 and 5.0.4. In both cases radius _efective is not updated when smearing is on.

RichardHeenan commented 2 years ago

Hmm, This becoming even more puzzling. I seem to have same behavior in 5.0.5rc2 and 5.0.4. In both cases radius _efective is not updated when smearing is on.

I strongly suspect that smearing and radius_effective/beta approx etc has never worked with smearing, In 5.0 release, radius_effective etc was broken anyway, my 5.0.1 release won't run due to my own gpu issue,, 5.0.2 release is not working.

gonzalezma commented 2 years ago

I concur with Richard's analysis. Using version 4.2.2 as reference, the hSDS_D2O_2p0_percent example in the tutorial and fitting with no smearing and with smearing (custom pinhole smear with 10%), I get the following:

  1. SasView versions from 5.0.1 to 5.0.5rc2 behave in the same way. The fits are correct (same chi^2 and parameters as in 4.2.2 with and without smear), but when the smear is applied the value in the interface for radius_effective is not updated. Without smear, the value is correctly updated (BTW, it's updated but not formatted as the other parameters).

  2. In SasView 5.0.0, radius_effective is never updated (even without smear). But the fits are still ok.

I would argue that this issue can be "downgraded" from the blocker status, as the problem is there since a long time and is only an interface issue. However, it is clearly annoying and confusing for a user. As suggested above, for the moment it could be simply described as a known issue, but I wonder how many users check those.

smk78 commented 2 years ago

This issue is fixed by the merge above so closing this ticket.