search

mantidproject / mantid

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

Script error for ISIS direct inelastic #16426

Closed martyngigg closed 8 years ago

martyngigg commented 8 years ago

From Jon Taylor:

I get an issue with Iliad, maybe in the production stuff for isis this is not a problem

line 1791 Direct energy conversion.py

KeyError: 'Property with name: nullify_negative_signal is not among the class properties '
  at line 56 in '/Users/jonathantaylor/Dropbox/SrFeO2/SrFeO2.py'
  caused by line 144 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/dgreduce.py'
  caused by line 525 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/DirectEnergyConversion.py'
  caused by line 717 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/DirectEnergyConversion.py'
  caused by line 1791 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/DirectEnergyConversion.py'
  caused by line 205 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/PropertyManager.py'
  caused by line 244 in '/Applications/MantidPlot.app/scripts/Inelastic/Direct/ReductionHelpers.py'

changing line 1791 to:

if prop_man.check_background: #and prop_man.nullify_negative_signal:

helps.

Tested on OSX

@abuts If this is not a problem for ISIS in production then we can probably drop this back to v3.8

abuts commented 8 years ago

'nullify_negative_signal' property has been introduced by #14456 (https://github.com/mantidproject/mantid/commit/553962481c9d450912198fd6eceb69d57cc807bc) on 18th of November in Mantid 3.6.0. It has been implemented as auto-property, automatically generated from an appropriate instrument parameter file, which has such property. As such it works with all recent Mantid versions given the IDF is also recent, so I can not reproduce the issue.

If local Mantid settings done in a way that old IDF is used or (may be, is it possible?) fully overwritten by old IDF stored in nxs file this error would occur.

I need a bit more information on when this error occurs to be able to reproduce the issue and think about the ways to deal with it.

martyngigg commented 8 years ago

@Jon-Taylor Can you provide any additional info for this?

NickDraper commented 8 years ago

@abuts If we are going to include this in R3.7 then we need to get the PR in today.

mducle commented 8 years ago

Tatiana also reports the same error... we can have a work-around in Direct_energy_conversion.py to check if this property exists and if not to assume it is false (e.g. default behaviour before November last year).

NickDraper commented 8 years ago

Thanks @mducle, we will take a look at this, could you give me the steps to reproduce the problem?

mducle commented 8 years ago

@martyngigg @NickDraper Sorry I was just with Tatiana trying to find out what was wrong.

It seems that an old version of Mantid on her Windows 7 laptop installed all the instrument definition into a user /AppData/Roaming/MantidInstall/mantid/instrument folder which is searched first. The newer definitions c:/MantidInstall/instrument is searched second (these paths were returned by mantid.api.ExperimentInfo.GetInstrumentFilename('MARI') and mantid.kernel.ConfigService.Instance().getInstrumentDirectories()). I have no idea where the first path (with the old instrument files) is defined - the preference dialogue only lists the second path (new definitions). I guess it could be a registry setting, but haven't looked.

Anyway, the work-around is:

diff --git a/scripts/Inelastic/Direct/DirectEnergyConversion.py b/scripts/Inelastic/Direct/DirectEnergyConversion.py
index f16b196..2adfa83 100644
--- a/scripts/Inelastic/Direct/DirectEnergyConversion.py
+++ b/scripts/Inelastic/Direct/DirectEnergyConversion.py
@@ -1788,7 +1788,7 @@ class DirectEnergyConversion(object):
         if prop_man.energy_bins: # It should already be a distribution.
             ConvertToDistribution(Workspace=result_ws)
         # nullify negarive signals if necessary
-        if prop_man.check_background and prop_man.nullify_negative_signal:
+        if prop_man.check_background and ('nullify_negative_signal' in dir(prop_man) and prop_man.nullify_negative_signal):
             zeroBg = CreateWorkspace(DataX='0,1',DataY=0,DataE=0,UnitX='TOF')
             result_ws=RemoveBackground(result_ws,BkgWorkspace=zeroBg,Emode='Direct',NullifyNegativeValues=True)
             DeleteWorkspace(zeroBg)
mducle commented 8 years ago

To reproduce the error, use the attached MARI_Parameters_Old.xml - everything works with MARI_Parameters_New.xml.

import MARIReduction_2016_1
MARIReduction_2016_1.iliad_mari('MAR21446',22,21281,0,0,0)

MariParameters.zip

MARIReduction_2016_1.py is included in the zip. Datafiles can be obtained from the archive (runs MAR21446 and MAR21281).

martyngigg commented 8 years ago

@mducle Just so I understand what has happened:

Is that about right?

In any case I will apply your fix but it sounds like we have uncovered and unintended side effect of instrument download mechanism.

mducle commented 8 years ago

@martyngigg Yes. That's about right. The /AppData/... folder only has the instrument files - I guess you mean that it's put there if you enable UpdateInstrumentDefinitions.OnStartup in Mantid.properties?

martyngigg commented 8 years ago

@mducle I think it's actually put there regardless of the setting of UpdateInstrumentDefinitions.OnStartup. The parameter just controls whether the download happens or not.

@NickDraper I think we will need to have a think what we can do to avoid this confusion in future versions.

NickDraper commented 8 years ago

Issue #16506 created to prevent this happening in the future

abuts commented 8 years ago

I am on AL now but its an important issue so I am trying to look into this. Still can not understand how it can happens, as Mantid pulls new IDF-s into /Appdata and uses it so both IDF-s should be correct. Communicating with Tatiana about this. She says that she have deleted old IDF from Appdata.

As I understand, @mducle fix is applied so it should be OK, but the issue is certainly deeper.

mducle commented 8 years ago

Mantid pulls new IDF-s into /Appdata and uses it so both IDF-s should be correct.

Does this only occur if UpdateInstrumentDefinitions setting is set to true? Maybe what happens is that when a new installation occurs this parameter is set to false, so the instrument definition is not updated. Is there a way to set this in MantidPlot? Or do you have to change Mantid.properties?

martyngigg commented 8 years ago

@mducle Yes, the downloading only occurs UpdateInstrumentDefinitions is set to true. It should always be switched on so I'm not 100% sure why they didn't download. The only think we could come up with is that it is trying to download them but is failing for some reason...

NickDraper commented 8 years ago

Alex,

Thanks for responding.

Martyn is putting Duc’s fix in, and I’m looking at adding a ClearCaches algorithm for 3.8 that should be able to get people out of this if it every happens again.

Regards, Nick Draper

From: abuts [mailto:notifications@github.com] Sent: 01 June 2016 14:43 To: mantidproject/mantid Cc: Draper, Nick (-,RAL,ISIS); Mention Subject: Re: [mantidproject/mantid] Script error for ISIS direct inelastic (#16426)

I am on AL now but its an important issue so I am trying to look into this. Still can not understand how it can happens, as Mantid pulls new IDF-s into /Appdata and uses it so both IDF-s should be correct. Communicating with Tatiana about this. She says that she have deleted old IDF from Appdata.

As I understand, @mduclehttps://github.com/mducle fix is applied so it should be OK, but the issue is certainly deeper.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mantidproject/mantid/issues/16426#issuecomment-222996070, or mute the threadhttps://github.com/notifications/unsubscribe/AA-FVbeGNLQKBVOyfZn9uWTdBT64AGJ9ks5qHYxugaJpZM4InQvk.

mducle commented 8 years ago

@martyngigg

It should always be switched on so I'm not 100% sure why they didn't download.

On my version of Mantid.properties, I have:

# Whether to check for updated instrument definitions on startup of Mantid
UpdateInstrumentDefinitions.OnStartup = 0
UpdateInstrumentDefinitions.URL = https://api.github.com/repos/mantidproject/mantid/contents/instrument

# Whether to check for newer mantid versions on startup
CheckMantidVersion.OnStartup = 0
CheckMantidVersion.GitHubReleaseURL = https://api.github.com/repos/mantidproject/mantid/releases/latest
CheckMantidVersion.DownloadURL = http://download.mantidproject.org

Tatiana also has the same...

martyngigg commented 8 years ago

@mducle Is that a downloaded nightly? I would only expect that on a development copy

mducle commented 8 years ago

Yes.

(Actually it was a downloaded 3.6.0 with self-compiled binaries installed on top, but I've just now checked with a fresh nightly download and it's the same).

(PS. Also checked with a fresh 3.6.0. install and it's still the same [i.e. UpdateInstrumentDefinitions.OnStartup = 0]... I saw something in CMakeList.txt which suggests it should be 0 for nightly and 1 for regular versions, but this doesn't seem to be passed on.)

martyngigg commented 8 years ago

Thanks, @mducle. It seems that there is indeed a bug in generating the properties file that has the downloading turned off when it should be on. I'm looking into it now.