Opened 6 years ago

Closed 6 years ago

#1188 closed defect (fixed)

fitpage hangs if change model while magnetism is on

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

Description

In 4.2, load some 2d data, send it to fitpage, select sphere model, turn on magnetic, do compute or fit, then change model to core_shell_sphere, the fitpage will hang with error below, suspect a gui problem.

File "C:\sasview42\sasview\src\sas\sasgui\perspectives\fitting\fitpage.py", line 1258, in _on_select_model

self.get_paste_params(saved_pars)

File "C:\sasview42\sasview\src\sas\sasgui\perspectives\fitting\basepage.py", line 3260, in get_paste_params

check = item[1]

Indexerror: list index out of range

Change History (15)

comment:1 Changed 6 years ago by richardh

The fitpage may recover if focus is changed elsewhere and then back again.
This was on Win7

comment:2 Changed 6 years ago by smk78

Verified.

Full traceback is:

Traceback (most recent call last):
  File "D:\SasViewDevelopment\sasview\src\sas\sasgui\perspectives\fitting\fitpage.py", line 89, in on_set_focus
    flag = check_data_validity(self.data) & (self.model is not None)
  File "C:\Anaconda2x64\envs\sasview\lib\site-packages\wx-3.0-msw\wx\_core.py", line 16712, in __getattr__
    raise PyDeadObjectError(self.attrStr % self._name)
wx._core.PyDeadObjectError: The C++ part of the FitPage object has been deleted, attribute access no longer allowed.
Traceback (most recent call last):
  File "D:\SasViewDevelopment\sasview\src\sas\sasgui\perspectives\fitting\fitpage.py", line 89, in on_set_focus
    flag = check_data_validity(self.data) & (self.model is not None)
  File "C:\Anaconda2x64\envs\sasview\lib\site-packages\wx-3.0-msw\wx\_core.py", line 16712, in __getattr__
    raise PyDeadObjectError(self.attrStr % self._name)
wx._core.PyDeadObjectError: The C++ part of the FitPage object has been deleted, attribute access no longer allowed.
Traceback (most recent call last):
  File "D:\SasViewDevelopment\sasview\src\sas\sasgui\perspectives\fitting\fitpage.py", line 1256, in _on_select_model
    self.get_paste_params(saved_pars)
  File "D:\SasViewDevelopment\sasview\src\sas\sasgui\perspectives\fitting\basepage.py", line 3260, in get_paste_params
    check = item[1]
IndexError: list index out of range

You seem to be able to do everything else in the program except get input focus back in the FitPage.

comment:3 Changed 6 years ago by richardh

ALSO the normal default magnetic parameters only appear if you change model with magnetism off, else they are all zero.

comment:4 Changed 6 years ago by richardh

AND changing the number of shells in a multiplicity model, or including an S(Q), whilst magnetism is on also does a list index out of range

Last edited 6 years ago by richardh (previous) (diff)

comment:5 Changed 6 years ago by smk78

Copied from ESS JIRA-1199:

@pkienzle says:

The bug comes about because of my foolish decision to use parameter 
names like M0:theta which are not proper python variables.  In this 
particular case, the parameters in the transfer string uses ';' as 
a parameter separator, which conflicts.
 
Simplest fix is to use ';' as a separator, and hope that there weren't 
other reasons to use ':'. (1/2 hr)

Better fix is to rename the parameters in sasmodels to use an 
underscore separator.  I suspect that having ':' in the parameter 
names will mean that we can't use constraint expressions for the 
magnetic parameters. I believe we should place the magnetic tag at the 
end of the parameter name (sld_M0, sld_solvent_M0) rather than the 
beginning, much like we do for polydispersity parameters in sasmodels.  
(2 hr)

Or we could do the fake attribute notation (sld.M0, sld_solvent.M0) 
like we do in the sasview model wrapper, but this will take longer to 
implement. (4 hr)

In sasview we will need page state to translate the name on reload.  
Do we care about saved magnetic projects at this point?
Version 0, edited 6 years ago by smk78 (next)

comment:6 Changed 6 years ago by smk78

  • Priority changed from minor to blocker

comment:7 Changed 6 years ago by smk78

Simple fix is available in https://github.com/SasView/sasview/pull/187

I'm inclined not to pull it so close to release.

comment:8 Changed 6 years ago by Paul Kienzle <pkienzle@…>

In 610ef23645ef28d7c1e8c87aab1e03c8cd123a93/sasmodels:

rename magnetic parameters to not contain colon. Refs #1188

comment:9 follow-up: Changed 6 years ago by smk78

@pkienzle says:

Actually, it was much easier to update sasmodels.  See sasmodels PR 84.

Probably better to not apply the sasview patch [PR187], since there is no good reason to 
change the separator.  And anyway, I should have created a separate PR for fixing the 
default magnetic parameters.

Still doesn't fix the problem with too many magnetic parameters shown.

comment:10 in reply to: ↑ 9 Changed 6 years ago by richardh

Replying to smk78:

@pkienzle says:

Actually, it was much easier to update sasmodels.  See sasmodels PR 84.

Probably better to not apply the sasview patch [PR187], since there is no good reason to 
change the separator.  And anyway, I should have created a separate PR for fixing the 
default magnetic parameters.

Still doesn't fix the problem with too many magnetic parameters shown. 

the latter has its own ticket #1189

comment:11 Changed 6 years ago by smk78

I have updated Known Issues in the release notes with the gist of this ticket.

The question facing us is whether to release as we are, or to pull @pkienzle's new sasmodels PR84 first?

comment:12 Changed 6 years ago by Paul Kienzle <pkienzle@…>

In bd547d022d7afce0c8a39ebfc0c2551bc9339186/sasmodels:

restrict magnetic parameters to visible shells. Refs #1188

comment:13 Changed 6 years ago by smk78

@wojciech says:

I’ve just merged PR84 and PR85 to sasmodels. 

I will do some testing on OSX both for 4.2 and 5.0 and if it doesn’t fail I will move 
a 0.98 tag to HEAD. If it fails, we can release with 0.98 as it is now.
 
@smk78: Can you independently test 4.2 on Win (after it is built), please? 

comment:14 Changed 6 years ago by smk78

  • Milestone changed from SasView 4.3.0 to SasView 4.2.0

comment:15 Changed 6 years ago by smk78

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

Tested 4.2.0 Build 439 on x64 W10.

With the caveat that the magnetic SLD defaults are all 10-6 even though the GUI says they should be in units of 10-6 anyway (the fix is addressed in #1004 for 4.3.0), merging PR's 84 & 85 seems to have fixed the bug reported in this ticket.

Did some other typical user operations as well. Didn't notice any obvious negative consequences of this fix.

Closing this ticket.

Note: See TracTickets for help on using tickets.