Replacing a workspace & instrument view exception

[Russell Taylor]

Ross Miller wrote:

I noticed what I think is a bug in MantidPlot: If you start up the live listener in either ‘Rename’ or ‘Restart’ mode, and then view the output workspace in the instrument view, everything is fine until the next run transition. At that point, you get an unhandled exception from the instrument view and the dialog says the workspace doesn’t exist.

I’m guessing this is because at the run transitions, we either delete or rename the original workspace and then create a new one that has the same name. I assume the instrument view just sees its pointer to the workspace go invalid.

I expect this is because the rename notification is not going out/getting picked up.

[Russell Taylor]

This is nothing specific to live data - it happens as well if you load a file, show the instrument and then reload that file into a workspace with the same name. So I've updated the title.

The stack trace that leads to the exception is:

InstrumentActor::getWorkspace() at InstrumentActor.cpp:220             
InstrumentActor::setDataIntegrationRange() at InstrumentActor.cpp:1,069     
InstrumentActor::setIntegrationRange() at InstrumentActor.cpp:374 
InstrumentActor::updateColors() at InstrumentActor.cpp:634                
InstrumentWindow::afterReplaceHandle() at InstrumentWindow.cpp:730 

In InstrumentWindow::afterReplaceHandle() the code is making a call into InstrumentActor that ultimately requires the workspace, though the workspace has changed and the pointer has not been updated.

[Russell Taylor]

This bug was introduced in ​https://github.com/mantidproject/mantid/commit/a71aef8ebc93714d500d0ead74fcac856a0b43e3#diff-2 under #8569. The commit message says that it's just a code cleaning refactoring, but it clearly did more than that as it changed behaviour (from creating a new InstrumentActor object to attempting to update the existing one).

[Russell Taylor]

Re #9419. Remove no-op statement.

The getWorkspace method will throw if the workspace is going to be null so this line is pointless.

Changeset: caae3a81e1b779ec3bfd68be79f2126377a94b56

[Russell Taylor]

Re #9419. Consolidate code that uses the workspace pointer.

Changeset: 7ae5e42f33e20b9d7876d8eb7877b8b588ee1074

[Russell Taylor]

Re #9419. Check whether workspace is actually the same one.

The code before was OK if the 'new' workspace was actually still the same one, but led to an exception if it was a different workspace just having the same name. In the latter case we need to recreate the InstrumentActor (as the code always did in the past).

Changeset: 53fdf98d105a780ab196e366e81efc8a7cfbf331

[Russell Taylor]

The problem is now solved by checking whether the workspace being replaced is actually still the same workspace or not (it would be for, e.g., a ConvertUnits into the same output workspace). Things were OK if it was still the same workspace, but if not the exception turned up. This was the case for the live data example given in the description and if you load over a displayed workspace. Another example would be rebinning an event workspace but not preserving events.

Test by having a play with the Instrument Window. Make sure you can reproduce the issue before applying the fix and that it's gone afterwards. Check that things are OK in some other situations where a displayed workspace is being updated.

[Peter Parker]

Merge remote-tracking branch 'origin/bugfix/9419_instrumentview_exception_on_replace'

Full changeset: ​1e75114eb7f95ed609ef4942ce747919a0766a48

[Peter Parker]

Could reproduce the problem with the following:

ws = Load("IRS21360.raw")
inst_win = getInstrumentView("ws")
inst_win.show()
ws = Load("IRS21360.raw")

The script ran fine once I merged in the branch.

We'd also been seeing crashes when doing multiple reductions in the ISIS SANS interface with the Instrument View open. These seem to have been cleared up as well.

[Peter Parker]

@Russell - I take it the scope of the MantidPlotInstrumentViewTest.py file is limited enough so that this edge case was not considered a reasonable thing to test? Or is the double loading of a file that would put us off here?

[Russell Taylor]

Truth be told I didn't think about there being a test for this, as there usually isn't for MantidPlot. I suppose it could be tested, though the failure case before the fix would presumably have been the test getting stuck which isn't great.

[Stuart Campbell]

This ticket has been transferred to github issue ​10262