Added the FlatCell algorithm for creating a workspace for ISIS SANS reduction of LOQ #40429
Conversation
RabiyaF
force-pushed
the
40299-create-flatcell-algorithm
branch
from
b679e70 to
c596faa
Compare
RabiyaF
force-pushed
the
40299-create-flatcell-algorithm
branch
from
24c3b5d to
3893c22
Compare
|
The logic for the flatcell algorithm should now work with event workspaces and produce an xml file with the detector ids that need to be masked. Like I mentioned in the meeting, I think the excel sheet had a mistake in indexing the High Angle Banks (HAB). So, I have tried to correct those in the algorithm. The rest of the steps should be the same. Because of this change the results are not exactly equivalent to the ones provided in the reference file (113953). I think this PR will benefit from a review at this stage. I have also updated the system tests to output the X values as detector IDs (as was requested in the meeting). If there is a need to add more unit tests then let me know (I looked into it but because of the private methods - I could not). |
MialLewis
left a comment
MialLewis
left a comment
There was a problem hiding this comment.
Thanks for this, looks like we're not too far off.
First round of comments.
| /// Execution code | ||
| void exec() override; | ||
| void execEvent(); | ||
| double mean(const std::vector<double> &values); |
There was a problem hiding this comment.
Mark methods that's don't mutate class member variables const
|
|
||
| // Validate the number of histograms in the Input WS | ||
| const size_t nHist = inputWS->getNumberHistograms(); | ||
| if (nHist == 0) { |
There was a problem hiding this comment.
You can probably combine these two nHost checks into one.
Would also be good to give some context around the 17776 number too.
|
|
||
| // The output is expected to have 17784 values (nHist+nMonitorOffset) | ||
| const int nMonitorOffset{8}; | ||
| const int totalDetectorIDs{17784}; |
There was a problem hiding this comment.
There's a lot of variables defined and numbers hardcoded related to LOQ pixel numbers.
Could we abstract these into a helper class that can be shared between algorithms if necessary?
| /** Computes the mean of the input vector | ||
| */ | ||
| double FlatCell::mean(const std::vector<double> &values) { | ||
| return std::accumulate(values.begin(), values.end(), 0.0) / static_cast<double>(values.size()); |
There was a problem hiding this comment.
If we're not mutating the item's in values, we can use the constant iterators cbegin, cend etc.
| WorkspaceFactory::Instance().create("Workspace2D", 1, totalDetectorIDs, totalDetectorIDs); | ||
| setProperty("OutputWorkspace", outputWS); | ||
|
|
||
| // Clone input before rebinning |
There was a problem hiding this comment.
Why clone before rebin, rather than just specifying a different output name?
| std::vector<double> unmaskedLAB(outY.begin() + LABStart, outY.begin() + LABStop); | ||
| std::vector<double> unmaskedHAB(outY.begin() + HABStart, outY.begin() + HABStop); | ||
|
|
||
| // Calculate the thresholds |
There was a problem hiding this comment.
Can we allow users to specify the threshold multipliers as an input property?
| const double maskingThresholdLAB = 1 + normStdLAB; | ||
| const double maskingThresholdHAB = 1 + (0.5 * normStdHAB); | ||
|
|
||
| // Mask the LAB and HAB |
There was a problem hiding this comment.
I suspect a user might want to play around with the thresholds to see what is appropriate for a given run.
Could you allow the user a checkbox option to either apply the mask directly to to generate a separate mask workspace?
| FlatCell::maskByThreshold(unmaskedLAB, maskingThresholdLAB); | ||
| FlatCell::maskByThreshold(unmaskedHAB, maskingThresholdHAB); | ||
|
|
||
| // Copy each of the vectors to outY |
There was a problem hiding this comment.
Again, can we avoid the copy?
| std::copy(unmaskedLAB.begin(), unmaskedLAB.end(), maskedOutY.begin() + LABStart); | ||
| std::copy(unmaskedHAB.begin(), unmaskedHAB.end(), maskedOutY.begin() + HABStart); | ||
|
|
||
| // Determine which detector IDs need to be masked |
There was a problem hiding this comment.
This section of code relates to generating the mask file.
Currently SANS generate mask files using the instrument view, which I believe uses the SaveMask algorithm to save a mask workspace as an XML:
I suggest the workflow be:
- Use
MaskDetectorsIfalgorithm to produce a workspace with applied mask. You can mask here conditionally based upon - Extract mask workspace from workspace using
ExtractMask. Output mask workspace to ads if user specifies. - Use
SaveMaskon the mask workspace to save file as XML - Output either the masked or unmasked workspace, depending on user input.
| using namespace Mantid::DataObjects; | ||
|
|
||
| void FlatCell::init() { | ||
| declareProperty(std::make_unique<WorkspaceProperty<EventWorkspace>>("InputWorkspace", "", Direction::Input), |
There was a problem hiding this comment.
I think your implementation would allow for the inputing of old non-event workspaces quite easily? It would be quite nice to be able to generate old mask workspaces to prove the logic.
If you make this a MatrixWorkspace property we could take both event and histo workspaces.
Perhaps come back to this at the end as a lower priority.
2 participants
Description of work
Closes #40299 .
To test:
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: