Make analysis reduction a Mantid algorithm #128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The unit test that existed before used a mock of mantid and of a table workspace to test. It is better practice to avoid mocking when possible, and this way the internal implementation of the unit test can change whilst maintaining the validity of the test.
Started work to make analysis routine into an object. Created folder to keep my work organised.
Most of the work consisted in including 'self' in functions and replace the previous 'ic' methods with 'self' methods
The first part of the routine up until the MS and Gamma corrections is now working, have not tested it yet.
Fixed second part of routine to use analysis object. Currently running in its entirety but have not checked tests
Added a system test for the new AnalysisReduction object. All system tests are passing. The system tests have a coverage of 95% of analysis_reduction.py and are very stringent, so it is fairly safe to say that this transition to OOP has not introduced any new bugs.
ajjackson
reviewed
Aug 2, 2024
ajjackson
reviewed
Aug 2, 2024
ajjackson
reviewed
Aug 2, 2024
Instead of fit the workspace in one go, and then create a table and the final workspaces, it makes more sense to populate the table and the final worksapaces row by row as the spectra are being fit row by row.
- Renamed functions - Changed functions to use self._row_being_fit instead of using the row as a function argument - Deleted functions made redundant due to previous commit
When the input workspace contains columns of zeros, these should be replaced by the fitted profile before doing the multiple scattering and gamma correction.
Made it easier to read and understand what the routine is doing when calculating the mean widths and intensity ratios. Put the multiple scattering and gamma corrections into its own function.
This work changes analysis_routines.py to be the "glue" between current interface and new oop interface. The idea is to keep the current interface for now and focus on turning the analysis rouinte into a Mantid algorithm. The analysis system tests were changed to account for this re-routing.
Rewrote routine for chaining two analysis routines together, known as the 'JOINT' procedure. This means that the resulting means and intensities from one analysis routine, usually backscattering, are used as the starting guesses for forward routine, and all widths except the lightest element (usually Hydrogen) are fixed. New procedure using the AnalysisReduction object is easier to read and less prone to bugs. Also checked that figures and result files are saved at correct location.
The new AanalysisReduction class is already being tested by test_analysis.py so the removed test was redundant.
Updated procedure to estimate H ratio to use new object of AnalysisReduction. The ratio is set to always be in relation to the lowest mass in the sample.
I added two functions to handle constraints with the new OOP interface, but promptly forgot that they are not necessary in the current interface as it is, so I commented them out for now, to be used in the future when the shift towards using the new interface happens
Made imports clearer, deleted useless comments and organized some sections into separate functions.
This commit finalizes the transition between the previous analysis routine using only functions and the new one using an object to run the routine. All of the intermediate files have been deleted and final routine was renamed to the existing analysis_reduction.py
Separated import statements by three groups of standard library, external imports and mvesuvio package imports
Implemented review suggestions
|
I've changed the vesuvio analysis reduction routine into it's own object, AnalysisReduction. This object is not a Mantid algortihm yet, but I would rather get this PR sorted out first and deal with the transition into a Mantid algorithm separately. Regarding the unit testing, my plan is to follow @MialLewis 's unit tests for the Calibration Mantid algorithm https://github.com/mantidproject/vesuvio/blob/main/tests/unit/calibration/test_calibrate_vesuvio_fit.py. @ajjackson What do you think? I suppose turning it into a Mantid algorithm does make the unit tests more verbose, but I don't think that is a good enough reason to avoid doing it? |
|
I agree that verbosity is not a particularly bad thing in testing. It is good to avoid making tests overly clever/complicated because usually there is no test of the test; sometimes that adds verbosity compared to an "elegant" (with subtle consequences) implementation. Anyway the patching/mocking in the file you linked to looks like routine stuff, and an appropriate way to implement fine-grained unit tests in code with a lot of interacting methods. |
This was referenced Aug 20, 2024
SilkeSchomann
requested changes
Aug 22, 2024
Uncomment functions to use in the future
GuiMacielPereira
force-pushed
the
make-analysis-reduction-algorithm
branch
from
444c5ce
to
68abc66
Compare
|
@GuiMacielPereira I think with your last commit you have re-introduced all the typos again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of work:
Currently the analysis scripts are based on a terrible design for running the algorithms.
The inputs are passed as a single object containing all the input arguments, which then are used by functions all the way down the reduction until the results are created as an object at the end of the routine.
Input validation is poor, the procedures defined are highly confusing to understand and the main analysis routine contains functions with many shared arguments that should be in principle contained within the same object.
Proposed Solution
Both the analysis and yfit routine should be made into a Mantid Algorithm.
The design means that certain arguments are easily available to function throughout the algorithm and it's simpler to keep track of the state as the algorithm progresses.
Dealing with inputs, input validation, constructing procedures and extracting outputs will be highly simplified and made robust.
In particular, VESUVIO has many diferent ways of chaining routines such that the fit parameters of profiles from backscattering detectors should be automatically be the initial guess for the fitting parameters of the forward scattering detectors. An OOP design will make this transition much smoother and whole lot more safe than it is currently.
Steps until Mantid algorithm:
analysis_reduction.pyto an object (non-Mantid yet)analysis_routines.pyto use new designTo test:
Fixes #127 .