Indirect QuickRun - DiffScan #19171

Merged
merged 7 commits into from Mar 31, 2017

Conversation

Projects
None yet
3 participants
@louisemccann
Contributor

louisemccann commented Mar 17, 2017

Added an algorithm to speed up the workflow for Indirect Inelastic diffraction reduction

To test:

If no access to ISIS ADS: Diff_inputs.zip

IndirectDiffScan(InputFiles=['26181','26182'],
                           Instrument='IRIS',
                           SpectraRange=[105, 112])

Fixes #19155 .

Release Notes have been updated


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@soininen soininen self-assigned this Mar 23, 2017

@@ -0,0 +1,278 @@
+from mantid.kernel import *

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

For Python 3, it might be worth starting this source file with

from __future__ import (absolute_import, division, print_function)
@soininen

soininen Mar 23, 2017

Contributor

For Python 3, it might be worth starting this source file with

from __future__ import (absolute_import, division, print_function)
+
+ # Instrument configuration properties
+ self.declareProperty(name='Instrument', defaultValue='',
+ validator=StringListValidator(['IRIS', 'OSIRIS']),

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

Would it make sense to try to use the instrument user has set if this property is not specified? The default instrument can be accesses by

config.getInstrument()
@soininen

soininen Mar 23, 2017

Contributor

Would it make sense to try to use the instrument user has set if this property is not specified? The default instrument can be accesses by

config.getInstrument()

This comment has been minimized.

@louisemccann

louisemccann Mar 24, 2017

Contributor

The stringListValidator means they have to select an option. Also the instrument scientists typically swap between data from IRIS and OSIRIS regularly, so it makes sense to have the option mandatory here rather than their scripts picking up the wrong run because they forgot to change the instrument config

@louisemccann

louisemccann Mar 24, 2017

Contributor

The stringListValidator means they have to select an option. Also the instrument scientists typically swap between data from IRIS and OSIRIS regularly, so it makes sense to have the option mandatory here rather than their scripts picking up the wrong run because they forgot to change the instrument config

This comment has been minimized.

@soininen

soininen Mar 24, 2017

Contributor

Is the value of this option actually compared against the loaded files, then? I think I saw the instrument name being read directly from a workspace somewhere in this algorithm.

@soininen

soininen Mar 24, 2017

Contributor

Is the value of this option actually compared against the loaded files, then? I think I saw the instrument name being read directly from a workspace somewhere in this algorithm.

This comment has been minimized.

@louisemccann

louisemccann Mar 24, 2017

Contributor

It gets passed to ISISIndirectDiffractionReduction where it is used for the instrument parameter file. It could be an idea to refactor _get_InstrRun() to just _get_run()

@louisemccann

louisemccann Mar 24, 2017

Contributor

It gets passed to ISISIndirectDiffractionReduction where it is used for the instrument parameter file. It could be an idea to refactor _get_InstrRun() to just _get_run()

+ unit = ('Temperature', 'K')
+ else:
+ logger.notice('Vertical axis is in run number')
+ unit = ('Run No', 'last 3 digits')

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

The 'last 3 digits' unit looks a bit confusing in MantidPlot's data view: 181last 3 digits. Could adding a space and/or parentheses help? Or maybe just have the entire run number there.

@soininen

soininen Mar 23, 2017

Contributor

The 'last 3 digits' unit looks a bit confusing in MantidPlot's data view: 181last 3 digits. Could adding a space and/or parentheses help? Or maybe just have the entire run number there.

+ # Add the new vertical axis to each workspace
+ scan_workspace.replaceAxis(1, y_ws_axis)
+
+ mtd.addOrReplace(self._output_ws + '_scan', scan_workspace)

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

Is this the final output of the algorithm? You should rather do it using

setProperty('OutputWorkspace', scan_workspace)

Also, in its current form, this is broken:

outWS = IndirectDiffScan(InputFiles=['26181','26182'],
                           Instrument='IRIS',
                           SpectraRange=[105, 112])

In the above, outWS = None while it should refer to scan_workspace.

If forcing a suffix upon the output workspace is really desired, one could consider having an OutputNamePrefix property or similar instead of OutputWorkspace. In this case the behavior and suffix added should be mentioned in the algorithm documentation.

@soininen

soininen Mar 23, 2017

Contributor

Is this the final output of the algorithm? You should rather do it using

setProperty('OutputWorkspace', scan_workspace)

Also, in its current form, this is broken:

outWS = IndirectDiffScan(InputFiles=['26181','26182'],
                           Instrument='IRIS',
                           SpectraRange=[105, 112])

In the above, outWS = None while it should refer to scan_workspace.

If forcing a suffix upon the output workspace is really desired, one could consider having an OutputNamePrefix property or similar instead of OutputWorkspace. In this case the behavior and suffix added should be mentioned in the algorithm documentation.

+ issues = dict()
+
+ # Validate spectra range
+ spectra_range = self.getProperty('SpectraRange').value

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

Might be worth checking that numbers in SpectraRange are non-negative as well.

@soininen

soininen Mar 23, 2017

Contributor

Might be worth checking that numbers in SpectraRange are non-negative as well.

+
+ print 'Workspace name: %s' % ws.name()
+ print 'Number of spectra: %d' % ws.getNumberHistograms()
+ print 'Number of bins: %s' % ws.blocksize()

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

Would it make sense to use the print() function in the examples? The users may need to move on to Python 3 at some point.

@soininen

soininen Mar 23, 2017

Contributor

Would it make sense to use the print() function in the examples? The users may need to move on to Python 3 at some point.

+QuickRuns
+~~~~~~~~~
+
+- :ref:`IndirectDiffScan <algm-IndirectDiffScan>` to to improve diffraction reduction workflow

This comment has been minimized.

@soininen

soininen Mar 23, 2017

Contributor

Double to and missing dot. Maybe explain a bit how this improves the workflow?

@soininen

soininen Mar 23, 2017

Contributor

Double to and missing dot. Maybe explain a bit how this improves the workflow?

@soininen

This comment has been minimized.

Show comment
Hide comment
@soininen

soininen Mar 30, 2017

Contributor

@louisemccann There seems to be some conflicts with master.

Contributor

soininen commented Mar 30, 2017

@louisemccann There seems to be some conflicts with master.

@soininen

This comment has been minimized.

Show comment
Hide comment
@soininen

soininen Mar 31, 2017

Contributor

The algorithm runs fine with the test data provided. :shipit:

Contributor

soininen commented Mar 31, 2017

The algorithm runs fine with the test data provided. :shipit:

@NickDraper NickDraper merged commit 334fe22 into master Mar 31, 2017

9 checks passed

ClangFormat Jenkins build pull_requests-clang-format 12444 has succeeded
Details
Doxygen Jenkins build pull_requests-doxygen 11771 has succeeded
Details
Flake8 Jenkins build pull_requests-flake8 3095 has succeeded
Details
OSX Jenkins build pull_requests-osx 13005 has succeeded
Details
RHEL7 + System Tests Jenkins build pull_requests-rhel7 12841 has succeeded
Details
Ubuntu + Doc Tests Jenkins build pull_requests-ubuntu 13447 has succeeded
Details
Ubuntu Python 3 Jenkins build pull_requests-ubuntu-python3 992 has succeeded
Details
Windows Jenkins build pull_requests-win7 13703 has succeeded
Details
cppcheck Jenkins build pull_requests-cppcheck 13430 has succeeded
Details

@NickDraper NickDraper deleted the 19155_indirect_quickrun_diffusion branch Mar 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment