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

HistogramData rollout: ConvFit.cpp to ResNorm.cpp #18767

Merged
merged 6 commits into from
Feb 14, 2017

Conversation

PranavBahuguna
Copy link
Contributor

This PR is part of refactoring effort to use the new interface of the HistogramData module.
See #17641 for details.

The following algorithms were converted to use HistogramData:

  • MantidQt/CustomInterfaces/src/Indirect/ConvFit.cpp
  • MantidQt/CustomInterfaces/src/Indirect/CorrectionsTab.cpp
  • MantidQt/CustomInterfaces/src/Indirect/IndirectDataReductionTab.cpp
  • MantidQt/CustomInterfaces/src/Indirect/IndirectSymmetrise.cpp
  • MantidQt/CustomInterfaces/src/Indirect/Iqt.cpp
  • MantidQt/CustomInterfaces/src/Indirect/IqtFit.cpp
  • MantidQt/CustomInterfaces/src/Indirect/ISISCalibration.cpp
  • MantidQt/CustomInterfaces/src/Indirect/ISISDiagnostics.cpp
  • MantidQt/CustomInterfaces/src/Indirect/ISISEnergyTransfer.cpp
  • MantidQt/CustomInterfaces/src/Indirect/ResNorm.cpp

No performance tests were added.

No algorithms show significantly improved performance.

Fixes #18755.


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.

@PranavBahuguna PranavBahuguna added Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period. labels Feb 8, 2017
@PranavBahuguna PranavBahuguna added this to the Release 3.10 milestone Feb 8, 2017
@KarlPalmen KarlPalmen self-assigned this Feb 13, 2017
@KarlPalmen
Copy link
Contributor

If I run indirect data reduction ISIS Energy Transfer after just loading file iris26176_graphite002_red.nxs, Mantid gets an unexpected exception Attempt to assign property (SpectrumMin) of incorrect type.

@PranavBahuguna
Copy link
Contributor Author

@KarlPalmen I tried doing the same thing on master and got the same result. This means that it isn't the result of any changes I made.

@PranavBahuguna
Copy link
Contributor Author

@KarlPalmen According to @louisemccann, this shouldn't work anyway as iris26176_graphite002_red.nxs is already reduced data. If you wish to test this, ensure you have access to the ADS and type in 26176, this will load a .raw file.

@KarlPalmen
Copy link
Contributor

I tried the suggestion and it worked. Other tests that I believe use the modified code also work. :shipit:

@peterfpeterson peterfpeterson merged commit 348d5cd into master Feb 14, 2017
@peterfpeterson peterfpeterson deleted the 18755_HistogramData_rollout_CF_to_RN branch February 14, 2017 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants