Algorithms testing for input/output workspace equivalence via name instead of pointer

[Russell Taylor]

Generally, algorithms should test whether the user has requested that the output workspace be the same as the input one via the pointer, not the name, as when called as a sub-algorithm the name may not be there. I recently fixed (#5860) SaveNexus, which was doing it wrong, and thought I'd look for other instances.

A search on getPropertyValue("InputWorkspace in algorithm source code finds the following. Not all these instances are necessarily incorrect, but I'm pretty sure some will be:

Algorithms/src/ChangeBinOffset.cpp:      if (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace"))
Algorithms/src/ChangeLogTime.cpp:    msg << "InputWorkspace \'" << this->getPropertyValue("InputWorkspace")
Algorithms/src/ConjoinWorkspaces.cpp:  AnalysisDataService::Instance().remove(getPropertyValue("InputWorkspace2"));
Algorithms/src/ConjoinWorkspaces.cpp:  AnalysisDataService::Instance().remove(getPropertyValue("InputWorkspace1"));
Algorithms/src/ConjoinWorkspaces.cpp:  AnalysisDataService::Instance().remove(getPropertyValue("InputWorkspace2"));
Algorithms/src/ConjoinWorkspaces.cpp:  declareProperty(new WorkspaceProperty<>("Output",getPropertyValue("InputWorkspace1"),Direction::Output));
Algorithms/src/ConjoinWorkspaces.cpp:  if (retval) AnalysisDataService::Instance().remove(getPropertyValue("InputWorkspace2"));
Algorithms/src/DiffractionEventCalibrateDetectors.cpp:      alg2->setPropertyValue("InputWorkspace", getPropertyValue("InputWorkspace"));
Algorithms/src/DiffractionFocussing2.cpp:  bool inPlace = (this->getPropertyValue("InputWorkspace") == this->getPropertyValue("OutputWorkspace"));
Algorithms/src/FilterByLogValue.cpp:  if (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace"))
Algorithms/src/FindDeadDetectors.cpp:        childAlg->setPropertyValue( "InputWorkspace", getPropertyValue("InputWorkspace") );
Algorithms/src/GetEi.cpp:                              getPropertyValue("InputWorkspace") );
Algorithms/src/MaskBinsFromTable.cpp:        maskbins->setPropertyValue("InputWorkspace", this->getPropertyValue("InputWorkspace"));
Algorithms/src/ModeratorTzero.cpp:  if (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace"))
Algorithms/src/MuonRemoveExpDecay.cpp:    if (getPropertyValue("InputWorkspace") != getPropertyValue("OutputWorkspace"))
Algorithms/src/NormaliseByCurrent.cpp:  if (getPropertyValue("InputWorkspace") != getPropertyValue("OutputWorkspace"))
Algorithms/src/Rebin.cpp:          subAlg->setPropertyValue("InputWorkspace", getPropertyValue("InputWorkspace"));
Algorithms/src/RenameWorkspace.cpp:  if (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace"))
Algorithms/src/ResetNegatives.cpp:    alg->setPropertyValue("InputWorkspace", this->getPropertyValue("InputWorkspace"));
Algorithms/src/ResetNegatives.cpp:      if (this->getPropertyValue("InputWorkspace") == this->getPropertyValue("OutputWorkspace"))
Algorithms/src/ResetNegatives.cpp:        setPropertyValue("OutputWorkspace", this->getPropertyValue("InputWorkspace"));
Algorithms/src/ResetNegatives.cpp:        alg->setPropertyValue("InputWorkspace", this->getPropertyValue("InputWorkspace"));
Algorithms/src/SassenaFFT.cpp:  const std::string gwsName = this->getPropertyValue("InputWorkspace");
Algorithms/src/ScaleX.cpp:      if (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace"))
Algorithms/src/ShiftLogTime.cpp:      msg << "InputWorkspace \'" << this->getPropertyValue("InputWorkspace")
Algorithms/src/StripVanadiumPeaks.cpp:  if (inputEvent && (getPropertyValue("InputWorkspace") == getPropertyValue("OutputWorkspace")))
Algorithms/src/SumRowColumn.cpp:  childAlg->setPropertyValue( "InputWorkspace", getPropertyValue("InputWorkspace") );
Crystal/src/AnvredCorrection.cpp:  bool inPlace = (this->getPropertyValue("InputWorkspace") == this->getPropertyValue("OutputWorkspace"));
DataHandling/src/LoadLOQDistancesFromRaw.cpp:    alg->setPropertyValue("Workspace", getPropertyValue("InputWorkspace"));
DataHandling/src/LoadSampleDetailsFromRaw.cpp:    g_log.error() << "Cannot retrieve InputWorkspace " << getPropertyValue("InputWorkspace");
DataHandling/src/LoadSampleDetailsFromRaw.cpp:    throw Exception::NotFoundError("Cannot retrieve InputWorkspace", getPropertyValue("InputWorkspace"));
DataHandling/src/SaveRKH.cpp:    << " Workspace: " << getPropertyValue("InputWorkspace") << "\n";
DataHandling/src/SaveToSNSHistogramNexus.cpp:    m_inputWorkspaceName = getPropertyValue("InputWorkspace");
MPIAlgorithms/src/BroadcastWorkspace.cpp:      g_log.fatal("InputWorkspace '" + getPropertyValue("InputWorkspace") + "' not found in root process");
WorkflowAlgorithms/src/EQSANSReduce.cpp:  const std::string inputWSName = getPropertyValue("InputWorkspace");
WorkflowAlgorithms/src/SANSSensitivityCorrection.cpp:  const std::string inputWSName = getPropertyValue("InputWorkspace");

[Russell Taylor]

Re #5864. Compare input/output workspace by pointer, not name.

Change a few places where a check for workspace same-ness was using the name rather than the pointer. This can fail to do what it should when an algorithm is being run as a subalgorithm.

Algorithms fixed: ChangeBinOffset, ModeratorTzero, NormaliseByCurrent & Rebin

Changeset: 31605562d4411fcbfbd24523da7b9b9dfcf2c138

[Russell Taylor]

I also verified that ChangeLogTime, which was captured in the grep, is OK - it's just a log message.

[Russell Taylor]

Re #5864. Fix more places where workspace name was being used

instead of (correctly) the pointer.

Changeset: 07e36b739add9611522beb194ff6c2142f20d5e7

[Russell Taylor]

Re #5864. More usage of workspace name instead of pointer eliminated.

Changeset: 3b20a45977384644977d6188c4824224b8a47312

[Russell Taylor]

With the exception of ConjoinWorkspace - which is a bit more challenging to untangle due to its direct use of the ADS - all of the places identified in the description have been dealt with. A few remain, but are valid uses of the name.

However, do a grep like getPropertyValue\(\.*Workspace and you see a whole bunch of other places that need fixing....

[Russell Taylor]

In fact, ConjoinWorkspaces is OK as a call to the ADS remove() method just gives a warning if the workspace isn't there.

[Russell Taylor]

Everything in the original ticket is sorted. I'll create another ticket for the next iteration to look at places shown up by the grep mentioned in comment:6.

How to test? Well, the rigorous way would be to make sure that all the algorithms can be successfully run as child algorithms with the input workspaces not being in the ADS. More realistically, just look at the code changes and observe that all tests are passing.

[Martyn Gigg]

Code changes make sense and the tests are all passing. Was the other ticket created?

[Russell Taylor]

Re #5864. Compare input/output workspace by pointer, not name.

Change a few places where a check for workspace same-ness was using the name rather than the pointer. This can fail to do what it should when an algorithm is being run as a subalgorithm.

Algorithms fixed: ChangeBinOffset, ModeratorTzero, NormaliseByCurrent & Rebin

Changeset: 31605562d4411fcbfbd24523da7b9b9dfcf2c138

[Russell Taylor]

Re #5864. Fix more places where workspace name was being used

instead of (correctly) the pointer.

Changeset: 07e36b739add9611522beb194ff6c2142f20d5e7

[Russell Taylor]

Re #5864. More usage of workspace name instead of pointer eliminated.

Changeset: 3b20a45977384644977d6188c4824224b8a47312

[Stuart Campbell]

This ticket has been transferred to github issue ​6710