Ticket #7351 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

SofQ is broken

Reported by: Alex Buts Owned by: Alex Buts
Priority: major Milestone: Release 2.6
Component: Direct Inelastic Keywords:
Cc: jon.taylor@…, nick.draper@… Blocked By:
Blocking: Tester: Martyn Gigg

Description (last modified by Martyn Gigg) (diff)

Take direct geometry reduced file (I tried ISIS MAPSReduction.nxs from system tests but they all seems have the same problem)

Run SofQ.

error:

Error in execution of algorithm SofQW: Workspace2D::getSpectrum, histogram number 1200 out of range 1200

Change History

comment:1 Changed 7 years ago by Alex Buts

  • Owner set to Alex Buts
  • Status changed from new to assigned

comment:2 Changed 7 years ago by Alex Buts

  • Status changed from assigned to accepted

comment:3 Changed 7 years ago by Alex Buts

refs #7351 This should fix it

Added the piece of code, which processes Ei log value in a way similar to SofQ3 does

Changeset: ca33d17c71855cf4af2d1cde4520aa5db1af845a

comment:4 Changed 7 years ago by Alex Buts

  • Status changed from accepted to verify
  • Resolution set to fixed

To tester:

Fixed SofQ should run in indirect mode without providing incident energy as input property if Ei log value is present on data file

(e.g. MARIREduction.nxs can be processed by SofQ without providing incident energy value EFixed)

if one deletes data logs (RemoveLogs) SofQ would request to have EFixed provided.

If Efixed provided is wrong (too small for the energy transfer, present in the data file) it generates meaningful error message.

comment:5 Changed 7 years ago by Russell Taylor

  • Status changed from verify to reopened
  • Resolution fixed deleted

comment:6 Changed 7 years ago by Alex Buts

  • Status changed from reopened to accepted

comment:7 Changed 7 years ago by Alex Buts

refs #7351 Trivial fix to compiler warning

Changeset: 76860622e893240b15dd4babbf8d2add8faea66e

comment:8 Changed 7 years ago by Alex Buts

  • Status changed from accepted to verify
  • Resolution set to fixed

comment:9 Changed 7 years ago by Gesner Passos

  • Status changed from verify to verifying
  • Tester set to Gesner Passos

comment:10 Changed 7 years ago by Gesner Passos

  • Status changed from verifying to reopened
  • Resolution fixed deleted

Two things:

Run this and the same 'wrong' output comes out.

Load(Filename=r'MAPSReduction.nxs',OutputWorkspace='MAPSReduction')
SofQW(InputWorkspace='MAPSReduction',OutputWorkspace='out',QAxisBinning='0.2,0.2,2.0',EMode='Indirect', EFixed=2.082)

Besides, it is not clear from the documentation that in direct mode, the ei from the run will override the one given by the user. (What I thing goes against the expected behavior. Values given by the user should override default values, do you agree?)

comment:11 Changed 7 years ago by Alex Buts

  • Status changed from reopened to accepted

2) Yes, I have not looked at Indirect reduction, so may be worth fixing it too. Indirect usually has energy on components, so user supplied energy is rare.

1) Doing Efixed on workspace overrides users Efixed I followed conversion noticed in SaveNXSPE, which used just before dealing with this ticket. The problem with user overrides Ei on workspace is that property remembers last used value. This means that user dealing with another workspace with another energy and not changing the energy in the algorithm window will provide wrong energy. This is why I think Efixed on workspace should override Efixed as property for direct conversion. For indirect it should be probably vise versa, as diffractometer set up to a specific energy which rarely changes, so user does not provides an energy without a reason. So, honestly, I do not know what way is better. Any decision of course should be well documented, so I would accept this change on this basis.

comment:12 Changed 7 years ago by Alex Buts

refs #7351 Extracted Common part of the SofQ codes into separate class

Changeset: c73febef98b7561978f42272aba395962a93aa03

comment:13 Changed 7 years ago by Alex Buts

refs #7351 fixed crash in PDFFourierTransform

which is not connected to SofQ in any way but prevents me from running unit tests on Algorithms.

Changeset: 2fb0ead633fbe561457207de6a880d6c5fb7dac2

comment:14 Changed 7 years ago by Alex Buts

refs #7351 Fixed ParameterMap failing in debug mode

after replacing double if statement It does not look like anything to do with SofQ but prevents me from running unit tests for algorithms.

Changeset: d430704403446076f46bcc743ba0b93b814c8b40

comment:15 Changed 7 years ago by Alex Buts

refs #7351 Fixing ExtractMaskToTable which fails test in debug mode

this is nothing to do with SofQ but prevents me from running Algorithms tests in debug mode.

Changeset: 4fc77fbf691aab7d6372f522e3113522ad8091d4

comment:16 Changed 7 years ago by Alex Buts

refs #7351 fixed crash in PDFFourierTransform

which is not connected to SofQ in any way but prevents me from running unit tests on Algorithms.

Changeset: 8e1094ceed3700800b6bb748fb7bfa47a26c35b9

comment:17 Changed 7 years ago by Alex Buts

refs #7351 Fixed ParameterMap failing in debug mode

after replacing double if statement It does not look like anything to do with SofQ but prevents me from running unit tests for algorithms.

Changeset: b3c51923444cab1ef5eae67ee196c2c144ba21c5

comment:18 Changed 7 years ago by Alex Buts

refs #7351 Fixing ExtractMaskToTable which fails test in debug mode

this is nothing to do with SofQ but prevents me from running Algorithms tests in debug mode.

Changeset: 5c93875e2d1ae9143b7c2b46a6e725150dfc07cc

comment:19 Changed 7 years ago by Alex Buts

refs #7351 Reverting commits related to #7454

Changeset: c74ce0e70175d0546e6f4286d5516963221f3cc1

comment:20 Changed 7 years ago by Alex Buts

  • Status changed from accepted to verify
  • Resolution set to fixed

After some elaboration I've decided that the best way of dealing with this stuff is to provide unified interface for all SofQ algorithms. This is why the part responsible for Efixed property and emode is extracted from all 3 algorithms and added as separate class to all of them.

I have also added check for negative ei in indirect mode, though the only situation when it can happens is when one processed direct data in indirect mode (fundamentally wrong)

Unit tests pass though I did not tried anything else.

There were unit tests failing in Debug mode, so I had to fix this to actually run them. All these commits have been extracted from this ticket and went into #7454

Last edited 7 years ago by Alex Buts (previous) (diff)

comment:21 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Description modified (diff)
  • Tester changed from Gesner Passos to Martyn Gigg

comment:22 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to reopened
  • Resolution fixed deleted

There are some compiler warnings that I assume are related to this ticket. Could you fix them first.

https://builds.sns.gov/view/Develop%20Clean/job/ornl_clean_rhel6_develop/2056/warnings17Result/?

While you are changing things could you look at the formatting of the SofQCommon code. There seems to be a lot of uneccessary whitespace, i.e line 10 & 11, line 55,56. And finally the indentation of line 27 of SofCommon.h is inconsistent.

comment:23 Changed 7 years ago by Alex Buts

refs #7351 Trivial compiler warnings.

Changeset: dd1177525c706a58b312b2b0864ed29b890171d8

comment:24 Changed 7 years ago by Alex Buts

refs #7351 Changes in formatting.

Changeset: 5e9e5a85d5143e4008c7af515fc8ad16ead69a3d

comment:25 Changed 7 years ago by Alex Buts

  • Status changed from reopened to verify
  • Resolution set to fixed

comment:26 Changed 7 years ago by Gesner Passos

  • Status changed from verify to verifying
  • Tester changed from Martyn Gigg to Gesner Passos

comment:27 Changed 7 years ago by Gesner Passos

  • Status changed from verifying to verify
  • Tester Gesner Passos deleted

Now I tried this one, because you said the users do not supply energy:

Load(Filename=r'MAPSReduction.nxs',OutputWorkspace='MAPSReduction')
SofQW(InputWorkspace='MAPSReduction',OutputWorkspace='out',QAxisBinning='0.2,0.2,2.0',EMode='Indirect')

it failed with this line:

RuntimeError: Cannot find EFixed parameter for component "PSD_TUBE_STRIP pixel;PSD_TUBE_STRIP pixel;PSD_TUBE_STRIP pixel;PSD_TUBE_STRIP pixel;". This is required in indirect mode. Please check the IDF contains these values.
  at line 2 in '/tmp/testscrip.py'
  caused by line 547 in '/apps/workspace/mantid_testing/bin/mantid/simpleapi.py'

But, before your changes, the script pass.

I'll not fail it, but I will return to the pool, as I think I'm not confident in the field to analyze the outputs.

comment:28 Changed 7 years ago by Alex Buts

MARI is Direct instrument, so processing it in Indirect mode is the way to have problem, though it may process it producing some rubbish.

In this particular case, without providing energy, it tries to get EFixed on components and rightly fails to get one. Providing energy as a property should make it pass.

Direct reduced data should have Ei property attached, and I believe this one is present in the MAPSReduction.nxs file.

Indirect are IRIS and OSIRIS and these two should work with the script suggested though I have not tried

Last edited 7 years ago by Alex Buts (previous) (diff)

comment:29 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester set to Martyn Gigg

comment:30 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to verify

Merge remote-tracking branch 'origin/bugfix/7351_SofQ_isBroken'

comment:31 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying

comment:32 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to closed

comment:33 Changed 7 years ago by Nick Draper

  • Component changed from Mantid to Framework

comment:34 Changed 7 years ago by Nick Draper

  • Component changed from Framework to Direct Inelastic

comment:35 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 8197

Note: See TracTickets for help on using tickets.