Ticket #10225 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

SNSPowderReduction is broken when PreserveEvents=False

Reported by: Martyn Gigg Owned by: Wenduo Zhou
Priority: blocker Milestone: Release 3.3
Component: Framework Keywords:
Cc: Blocked By:
Blocking: Tester: Vickie Lynch

Description

It was broken in this ​commit with the addition of a logging statement that asks for the number of events (​https://github.com/mantidproject/mantid/commit/342aca3e08d3ec21be2627f2d380f85b8bb72bfb#diff-1b114ee94e8df86b22e2cac0469acd8bR660)

This has caused the ​MPI system tests to fail as the PreserveEvents flag is force to false for an MPI build.

This script demonstrates the problem (based on the current master)

cal_file   = "PG3_FERNS_d4832_2011_08_24.cal"
char_file  = "PG3_characterization_2012_02_23-HR-ILL.txt"

# reduce a sum of runs - and drop it
SNSPowderReduction(Instrument="PG3", RunNumber=[9829,9830], Extension="_event.nxs",
                   Sum=True, # This is the difference with the next call
                   PreserveEvents=False, VanadiumNumber=-1,
                   CalibrationFile=cal_file,
                   CharacterizationRunsFile=char_file,
                   LowResRef=15000, RemovePromptPulseWidth=50,
                   Binning=-0.0004, BinInDspace=True, FilterBadPulses=True,
                   SaveAs="gsas", OutputDirectory=os.path.expanduser("~/"),
                   FinalDataUnits="dSpacing")

Change History

comment:1 Changed 6 years ago by Martyn Gigg

  • Status changed from new to assigned

comment:2 Changed 6 years ago by Wenduo Zhou

  • Status changed from assigned to inprogress

comment:3 Changed 6 years ago by Wenduo Zhou

Fixed the bug. Refs #10225.

Changeset: d62f98e12748c5cf917a6a7ecb5a4708cd882629

comment:4 Changed 6 years ago by Wenduo Zhou

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

comment:5 Changed 6 years ago by Martyn Gigg

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

comment:6 Changed 6 years ago by Martyn Gigg

  • Status changed from verifying to reopened
  • Resolution fixed deleted

The fix does cure the problem, however the method for checking whether the workspace is an EventWorkspace is a little non-standard.

Can we replace line ​#664 with

if isinstance(temp, IEventWorkspace):

as this is more standard Python and I don't see the need for the first self.preserveEvents is True check as surely we only need to check whether we have an event workspace?

Last edited 6 years ago by Martyn Gigg (previous) (diff)

comment:7 Changed 6 years ago by Martyn Gigg

I have just another place where the .__class__ is used where it doesn't need to be:

  • Line ​#251 can be replaced with if isinstance(returned, list):

I think it would be good to clean this up now too.

comment:8 Changed 6 years ago by Michael Reuter

We need to get this cleaned up so that we can get the MPI build back in order.

comment:9 Changed 6 years ago by Wenduo Zhou

  • Status changed from reopened to inprogress

Refs #10225. Replace with isinstance.

On branch bugfix/10225_snspdred_mpi_failure Changes to be committed:

modified: Framework/PythonInterface/plugins/algorithms/SNSPowderReduction.py

Changeset: cd3e78840d95a629898e54d5341187e139c5fd2e

comment:10 Changed 6 years ago by Wenduo Zhou

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

For tester

Check the change, unit tests and system tests, especially the MPI one.

comment:11 Changed 6 years ago by Vickie Lynch

  • Status changed from verify to verifying
  • Tester changed from Martyn Gigg to Vickie Lynch

comment:12 Changed 6 years ago by Vickie Lynch

  • Status changed from verifying to closed

Merge remote branch 'origin/bugfix/10225_snspdred_mpi_failure'

Full changeset: ​4a9faf6b86246c7846dba3d33c5839741d064f6d

comment:13 Changed 6 years ago by Vickie Lynch

Merge remote branch 'origin/bugfix/10225_snspdred_mpi_failure'

Full changeset: ​4a9faf6b86246c7846dba3d33c5839741d064f6d

comment:14 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue ​11067

Note: See TracTickets for help on using tickets.