Opened 4 weeks ago

Closed 2 weeks ago

#1183 closed defect (fixed)

Test from creating new model reset all parameters to default in all open FitPages

Reported by: butler Owned by:
Priority: blocker Milestone: SasView 4.2.0
Component: SasView Keywords:
Cc: Work Package: SasView Bug Fixing

Description

While testing fix for ticket #1173 a new bug became clear which explained one user's extreme frustration in a recent email stating among other things:

I also wanted to change a little the model, as in some contrast my sponge phase S(q) has 2 bumps instead of 1. When I did it, it showed me an error (same as happened with other models and we talked about it in previous e-mails) and it set all the fitting windows I had opened to the default values. So basically I lost most of the fittings I did that day, except the ones I had in pdf.

The only exception seems to be that FitPage1 behaves somewhat differently than the others though I have not pinned down the exact nature of that. It seems that in that case the parameters are usually also reset to defaults but the graph is not updated (all graphs associated with the other tabs are immediately updated) and in fact may generate a new window?.

NOTE: I checked this was the case not only with plugins but a mixture of standard built in models as well.

Change History (16)

comment:1 Changed 4 weeks ago by smk78

How does one reproduce this bug?

comment:2 Changed 4 weeks ago by butler

Open fitpage do a fit. Open a second do a fit, open a third do a fit ….as many as you like. then try to create a new model or a sum|multiply (should also happen with advanced model editing). Create model. Click "apply" which launches the testing that has caused problems in the past (now fixed as far as my testing goes) and all the parameters in all the fitpages should get reset to their defaults and graphs updated —- except for tab one which appears to behave differently.

Talking to @pkienzle It may be that at least one tab has to have at plugin model loaded as he thinks this may be related to trying to deal with people changing a model that is already being used.

Also not sure one needs a fit. I believe you can get this but just opening a number of fitpages and loading models and changing the parameters (again not sure if one of them needs to be a plugin or not).

comment:3 Changed 4 weeks ago by pkienzle

Look in src/sas/sasgui/perspectives/fitting/fitting.py(337) update_combo_box().

This walks all the pages, updating the form factor box if it is a custom model and always resetting the structure factor box. The latter operation may be the culprit. In particular, the following change to line 379 seems to hide the problem:

page._on_select_model()  =>   page._on_select_model(keep_pars=True)

I'm not sure what happens when we have a custom model structure factor, but I suppose we would need to repopulate the structure box with the new result.

comment:4 Changed 4 weeks ago by pkienzle

The _on_select_model() call triggers a recalculation of the model. Because it is happening every time the fit page contains a structure factor box, that means the model will be recalculated even if it is not a plugin model. If it is a plugin model and it can have a structure factor, then the model will be recalculated twice.

Ideally we would only recalculate if the form factor or structure factor plugin for the fit page were modified. Since we don't keep track of exactly which plugins changed on reload (though sasmodels could track that…), the next best would be to recalculate only those pages which contain plugin models, either as form factor or structure factor.

This should be fixed before closing this ticket, or a new ticket should be created.

comment:5 Changed 4 weeks ago by butler

As described in the pull request, am guessing we should try the above next as the current fix is still causing problems. Copying below the comments from the pull request:

better as you don't immediately lose the work BUT will confuse the heck out of a user so may have effect of losing their work. Here is the test performed and the results:

I loaded various data from the test folder and sent one by one to fitting tabs. There were 6 fitting tabs in all though the 6th was labelled 7 because on tab 6 I accidentally loaded a data set I'd already used and not the one I wanted so deleted the graph and then the tab. The models used were:
Tab 1 = sphere no SQ
Tab 2 = core-shell (shell sld set equal to solvent sld) no SQ
Tab 3 = Gunier Porod —> NO SQ OPTION on this one
Tab 4 = Cylinder no SQ
Tab 5 = Sphere * SQ (hmsa — no salt)
Tab 7 = Plugin model (sponge using L3_porcar_el_al)

with Tab 7 on top, I opened "create new model" and made a test model using the tool tip example exactly though I did fail to see the ":" at the end of the if statement so originally Apply failed and nothing happened to the open fit tabs. Once it passed, all the fit tabs got updated. Results was that in ALL CASES the parameters on the tab were not affected, but also in ALL cases but one (the one with an actual S(Q) - see later) the graphs were updated using the default parameters. Clicking on compute redrew the graphs with the proper parameters. NOTE: this included the model with no SQ option!

Tab 7 which was on top had the GUI layout screwed up so difficult to read - switching to tab 6 and back cleared it up.

For Tab 5 with the P*S model neither the parameters on the tab OR the graph were redrawn - However a NEW graph popped up with the model only using the default parameters (i think). Worse, changing parameters and hitting either fit or compute only changed the model drawn in the new graph, while oddly hitting return in one of the parameter boxes updated the model curve in both graphs.


This was followed by another test and reported as:

Noticed that I had several other SasView windows open and worried that there may have been some polution that tainted the results so redid with slightly different data sets. Result was the same except that the P*S did not generate a new graph. Also, the plugin tab was completely unusable and illegible and going off to different tabs did not help this time. choosing the model in the dropdown did properly redraw the tab but of course rest the parameters to default. So comments above remain valid I believe

comment:6 Changed 3 weeks ago by smk78

Just been playing with this myself (Win64 Build 432). This is BAD!

The gist is, that if you create a Sum|Multi plugin when you already have populated FitPage's, as soon as you click OK after it runs the plugin test, SasView will reset ALL FitPage parameters to their defaults.

It doesn't matter which FitPage you were in when you went to create the plugin.

You don't need to have models with an S(Q).

You don't need to have models with an existing plugin.

You don't need to create a plugin with a model that is already applied in a FitPage.

Here's far simpler, quicker, demo than @butlers:

Start SasView
Load Data > select the 3 AOT datasets from test\1d_data
Send them to Fitting
Go to each FitPage > select a sphere model > check scale, background & radius > Fit
Now go Fitting > Plugin Model Operations > Sum|Multi
Create any plugin you like > Click Apply to run the test
Now watch the FitPage behind the Easy Sum|Multi window as you click OK

If you create the plugin before you start filling FitPage's you are fine.

comment:7 Changed 3 weeks ago by smk78

Just tried my test above in 4.1.2.

The FitPage's are unaffected.

comment:8 Changed 3 weeks ago by butler

I think almost all of this has been fixed in SasView pull request 85 (PR build 49 I think it is if you want to download the installer). The extensive current testing results is documented in the comments there. Unfortunately the remaining issues is probably the most common use case so if not quickly fixable the question of whether or not to release anyway will arise.

comment:9 Changed 3 weeks ago by smk78

Just switched to ticket-1183-parameter-reset branch and tried the same test.

After a worrying couple of seconds when all the parameter boxes refreshed (just like they do when they are about to be reset) I can confirm that the FitPages are actually unaffected. The fix in this branch seems to work.

comment:10 Changed 3 weeks ago by butler

Did you check the notes on the PR? I found that the plots associated with a 4.2 or 4.1.2 single model plugin (i.e. NOT a sum|multiply model plugin) still gets redrawn using the default parameters even though the FitPage keeps the parameters. This means that at least you can recover by clicking the compute button on that page but still ….

Or does that not happen for you?

Last edited 3 weeks ago by butler (previous) (diff)

comment:11 Changed 3 weeks ago by smk78

I did. Though the notes on the PR are actually quite confusing, since they either talk of conserving default parameters or replotting using DEFAULT parameters…

But I have just tried the same test also loading a non-library plugin (nanodisc_simple from the Marketplace) into a 4th FitPage. All is good. Already fitted parameters are preserved.

Can you clarify what to do to repeat the behaviour you see?

comment:12 Changed 3 weeks ago by butler

So Each "FitPage" in SasView really involves TWO windows: the tab with all the choices and buttons and the associated plot that is the graphic representation of the model and parameters chosen. Both have to update when a change is made to the model (some cases are obvious but think about a fit: both the parameters on the tab and the plot need to update with the final result)

So what you were seeing during that "couple of seconds of worrying" is some redrawing going on (don't ask … long story:-)

I spent about 4 hours looking at as many permutations as possible because it turns out not all of them run through the same code. The one remaining issue I see is that in one case the two windows update with different parameters and are no longer in synch:

  • take a 4.2.0 stand alone plugin (not a sum:multiply). I was creating them with "create new model" to make sure it was a 4.2.0 model, but my test suggested a 4.1.2 plugin will also exhibit the behavior (but not a 3.1.2 plugin which behaves perfectly fine)
  • Change the parameters significantly so that the plot is VERY different (easy to see the change visually)
  • do whatever you do to trigger the redraw (can be creating a new model, a sum model, or deleting an existing plugin (from SasView of course not the OS). At the end the parameters on the tab should be conserved … However for me the plots always update to the figure you would expect from the default parameters. If you go back to the associated fit tab and click on compute the plot will update again and now the curve should match the FitPage again.

Again I stress that this is only the associated plot window and is only true if the tab/plot are for a 4.2 or 4.1.2 standalone plugin.

comment:13 Changed 3 weeks ago by smk78

Just did this:

  • Started 4.1.2 and used sum|multi to create fractal+gaussian_peak. Called it "test". Closed 4.1.2.
  • Switched to ticket-1183-parameter-reset branch and ran locally.
  • Loaded AOT_Microemulsion-Drop_Contrast & ISIS_98929 to FitPages.
  • Selected sphere-"sphere" and plugin-"test" models, respectively.
  • Fitted scale, background & radius in "sphere" model, then added 1.0 to background to make it different.
  • In "test" model, set p1_scale=0.0, fitted p2_background, p2_peak_pos, and p2_sigma, then added 1.0 to p2-background to make it different.
  • Went Fitting > Plugin Operations > Sum|Multi and created "testb" (fractal+broad_peak). Apply. Ok. (ie, to try and force a redraw).

Fit parameters in the two FitPages have been preserved and so have both plots (in their deliberately offset form): they have NOT been reset to the model defaults.

  • Loaded ISIS_98929 to !FitPage3 with "testb" model.

All still good.

  • Deleted "test" model.

FitPage using "test" goes to no-model state (and popup appears telling you to select a model), but graph is preserved with last "test" theory calculation (guess there is a question as to whether that is desired behaviour…). But other graphs and FitPages are unaffected.

comment:14 Changed 3 weeks ago by pkienzle

I was seeing the model replotted with default parameter values even though the page itself was showing the modified parameter values, but only for plugin models. I updated the PR in sasview to fix this.

I added a new PR in sasmodels to improve the caching: currently it is redrawing all plugin models even if the module hasn't changed. It also detects changes to code on the source=["file.c", ...] list. This PR is not required to fix the ticket. I'll leave it to others to decide if it should be applied before the 4.2 release.

comment:15 Changed 3 weeks ago by smk78

@butler says in SasView PR185:

Functionality checked with all kinds of models, fits and theories -- not as extensive as 
before but all tabs and associated plots remain pegged to current parameters and do not 
get reset to defaults.

Also checked that if model in tab is changed, by adding a parameter and or changing the 
function the model gets updated on the page however if only the defaults are changed the 
tab and plot do NOT update to the new default but stay with what they were already set 
at -- so desired behavior.

Finally deleting a model that is on a tab seems to cleanly reset the FitPage to a blank 
FitPage and asks that a model be chosen. NOTE: the plot remains on the screen but this 
is consistant with current behavior that leaves plots on the page even when the 
associated tab is killed.

Somebody else needs to check code for danger signs but finally this nasty bugs seems 
fixed except for the annoying redraw of all the plugin tabs/plots which can take time 
and provide scary flashes if the page is full of plugin tabs. That may be fixed by 
PR83 in sasmodels though. Approving this request now from functionality point of view
@butler says in Sasmodels PR83:

Seems to work as advertized - if the code review passes this can be merged.

Have also just pulled https://github.com/SasView/sasview/pull/185 and https://github.com/SasView/sasmodels/pull/83 , built and run locally on Win64. Repeated my test described above. No issues observed. So also recommend merging of both PR's after code has been reviewed.

comment:16 Changed 2 weeks ago by butler

  • Resolution set to fixed
  • Status changed from new to closed

The merge occurred without the magic "closes" verbiage, so closing this manually.

Note: See TracTickets for help on using tickets.