Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indirect QuickRun - add diffusion option #18845

Closed
wants to merge 6 commits into from

Conversation

louisemccann
Copy link
Contributor

EnergyWindowScan has been extended to include diffraction reduction and renamed to IndirectReductionAndAnalysis to better represent what the algorithm does.

To test:
InputFiles: Diff_inputs.zip

1
Interfaces>Indirect>Diffraction
Input: 26181
(this should find the appropriate run in the data archive otherwise browse to IRS26181 in files provided)
Ensure Instrument = ISIS and mode = diffspec
Press Run

2

IndirectReductionAndAnalysis(InputFiles='26176',
                             Instrument='IRIS',
                             Analyser='graphite',Reflection='002',
                             SpectraRange='3, 50',
                             ElasticRange='-0.5, 0',
                             InelasticRange='0, 0.5',
                             GroupingMethod='All',
                             MSDFit=False,
                             Diffraction=True,
                             DiffractionSpectraRange=[105,112])

Ensure irs26176_diffspec_red is the same for both methods

Fixes #18223 #12270

Release Notes


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.

@louisemccann louisemccann added Indirect/Inelastic Issues and pull requests related to indirect or inelastic Component: Python labels Feb 14, 2017
@louisemccann louisemccann added this to the Release 3.10 milestone Feb 14, 2017
@samueljackson92 samueljackson92 self-assigned this Feb 15, 2017
@samueljackson92
Copy link
Contributor

samueljackson92 commented Feb 16, 2017

@louisemccann Question: EnergyWindowScan is already an algorithm in 3.9 right? So by renaming the algorithm to IndirectReductionAndAnalysis does this break the Python API? EnergyWindowScan is pretty niche, but shouldn't we deprecate for a version then remove?

One option would be to just point the deprecated EnergyWindowScan at the new algorithm.

diff_red_alg.setProperty('GroupingPolicy', self._grouping_method)
diff_red_alg.setProperty('OutputWorkspace', self._diff_workspace)
diff_red_alg.execute()
self.setProperty("DiffractionWorkspace", diff_red_alg.getProperty("OutputWorkspace").value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the stuff under the if statement could be refactored into a method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I'd go for one method per algorithm i.e. something like self.run_diffraction_reduction(), self.run_msg() ...

Copy link
Contributor

@samueljackson92 samueljackson92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See suggested changes above, or come talk to me if you disagree.

@SpencerHowells
Copy link

We need EnergyWindowScan for use by the Interface in #18216

@samueljackson92
Copy link
Contributor

samueljackson92 commented Feb 20, 2017

@SpencerHowells I agree the algorithm needs to exist. My problem is with renaming. Renaming would potentially break any user scripts that we have no control over that use the current name. Hence we should really deprecate the name EnergyWindowScan for one version of Mantid, then remove any reference of the algorithm called EnergyWindowScan in favour of IndirectReductionAndAnalysis.

@louisemccann
Copy link
Contributor Author

It has been decided that EnergyWindowScan and the DiffractionScan should be 2 different algs so I am going to close this.

@louisemccann louisemccann deleted the 18223_indirect_add_diffusion_to_quickrun branch March 14, 2017 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Indirect/Inelastic Issues and pull requests related to indirect or inelastic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants