mantidproject / mantid

Main repository for Mantid code
https://www.mantidproject.org
GNU General Public License v3.0
207 stars 121 forks source link

Improve scripting of workspace history for algorithm calls with StoreInADS=False #33252

Open DannyHindson opened 2 years ago

DannyHindson commented 2 years ago

If you take a workspace that has been generated by the ISIS Powder diffraction scripts and script the workspace history it doesn't run through properly (eg 98533-ResultD.zip). This is because of the following code from focus.py:

def _divide_one_spectrum_by_spline(spectrum, spline, instrument):
    rebinned_spline = mantid.RebinToWorkspace(WorkspaceToRebin=spline, WorkspaceToMatch=spectrum, StoreInADS=False)
    if instrument.get_instrument_prefix() == "GEM":
        divided = mantid.Divide(LHSWorkspace=spectrum, RHSWorkspace=rebinned_spline, OutputWorkspace=spectrum,
                                StoreInADS=False)
        complete = mantid.ReplaceSpecialValues(InputWorkspace=divided, NaNValue=0, StoreInADS=False)
        # crop based off max between 1000 and 2000 tof as the vanadium peak on Gem will always occur here
        return _crop_spline_to_percent_of_max(rebinned_spline, complete, spectrum, 1000, 2000)

    divided = mantid.Divide(LHSWorkspace=spectrum, RHSWorkspace=rebinned_spline,
                            StoreInADS=False)
    complete = mantid.ReplaceSpecialValues(InputWorkspace=divided, NaNValue=0, OutputWorkspace=spectrum)

    return complete

The algorithm calls gets scripted as follows:

RebinToWorkspace(WorkspaceToRebin='spline_bank_1_1', WorkspaceToMatch='focused_ws-1', OutputWorkspace='rebinned_spline')
Divide(LHSWorkspace='focused_ws-1', RHSWorkspace='__TMP0000020CAACF86E0', OutputWorkspace='divided')
ReplaceSpecialValues(InputWorkspace='__TMP0000020CAACF41D0', OutputWorkspace='focused_ws-1', NaNValue=0)

The __TMP workspace doesn't exist

DannyHindson commented 2 years ago

The StoreInADS=False doesn't get saved in the workspace history so this may be a problem as well as the actual scripting

DannyHindson commented 2 years ago

I've made a change to sort this out. Doesn't seem especially urgent to fix in 6.3 though - particularly because the code change seems a bit risky in that it changes the validation on workspace properties so that output workspace properties don't have to have a name. I'll put this through as a PR again main for 6.4 I think

martyngigg commented 2 years ago

Is it worth us opening this fix for main this time?

I'm assuming the change is to store the history whenever it has a name? I'm also wondering if we start to break the rule that names only exist in the ADS. We currently generate the tmp name in the workspace property I think. Maybe we should generate the name in the workspace constructor as just the string of the memory address?

I'll roll this to 6.5S1.