Group workspace masking error in MaskDetectors #18417
Conversation
In some cases the hardware grouping can cause a workspace to have more than on detector per spectrum. When we have a mask where each spectrum corresponds to a detector and the workspace to be masked has a smaller number of spectra we should append the indicies to the detectorList.
This adds the ability to pass a workspace which is not a mask workspace but a list of spectra corresponding to detectors to be masked. This is useful in the case where the number of spectra do not match but the number of detectors do, i.e. when some grouping has been applied to the workspace.
samueljackson92
added
the
Framework
| << prevMasking->getNumberHistograms() | ||
| << " histograms. \n"; | ||
| throw std::runtime_error("Size mismatch between two input workspaces."); | ||
| if (nHist > WS->getNumberHistograms()) { |
There was a problem hiding this comment.
This if does not fit the comment above it
There was a problem hiding this comment.
We also need to consider what should happen if it is <, probably handle it this way
| if (instrument && maskInstrument && | ||
| maskInstrument->getNumberDetectors() == | ||
| instrument->getNumberDetectors() && | ||
| nHist == instrument->getNumberDetectors()) { |
There was a problem hiding this comment.
I don't think we should have the number of detectors == nhist restriction.
| auto maskInstrument = prevMasking->getInstrument(); | ||
| if (instrument && maskInstrument && | ||
| maskInstrument->getNumberDetectors() == | ||
| instrument->getNumberDetectors() && |
There was a problem hiding this comment.
if the number of detectors in the instruments do not match we should just warn but continue, not refuse.
| appendToDetectorListFromWS(detectorList, WS, prevMasking, | ||
| ranges_info); | ||
| } else { | ||
| // We have more spectra in the mask than in the workspace and we also |
There was a problem hiding this comment.
So we should only refuse if one of the WS's does not have an instrument.
| throw std::runtime_error( | ||
| "Size mismatch between two input workspaces."); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This else should be more explicit of when it is being called (when workspace indices match)
|
|
||
| for (size_t i = 0; i < maskWs->getNumberHistograms(); ++i) { | ||
| if (maskWs->y(i)[0] == 0) { | ||
| auto id = maskWs->getDetector(i)->getID(); |
There was a problem hiding this comment.
There is a methods that will return a vector of all of the IDs (i think getIDs) that will allow this to work for none 1-1 mapping.
|
@NickDraper Please review again when you have time. |
| if (nHist == WS->getNumberHistograms()) { | ||
| appendToIndexListFromWS(indexList, maskWs, rangeInfo); | ||
| // Check they both have instrument and then use detector based masking | ||
| } else if (instrument && maskInstrument) { |
There was a problem hiding this comment.
This should check the instrument->name, not pointer address, but it also should only log as warning if they do not match, still go ahead and apply the masking.
| g_log.warning() << "Number of detectors does not match between " | ||
| "mask workspace and input workspace"; | ||
|
|
||
| appendToDetectorListFromWS(detectorList, WS, maskWs, rangeInfo); |
There was a problem hiding this comment.
We should log at info that the masking is being applied by detector ids
| // Check the provided workspace has the same number of spectra as the | ||
| // input, if so assume index list | ||
| if (nHist == WS->getNumberHistograms()) { | ||
| appendToIndexListFromWS(indexList, maskWs, rangeInfo); |
There was a problem hiding this comment.
We should log at info that the masking is being applied by workspace index
| forceDetIDs) { | ||
| extractMaskedWSDetIDs(detectorList, maskWs); | ||
| } else { | ||
| appendToIndexListFromMaskWS(indexList, maskWs, rangeInfo); |
There was a problem hiding this comment.
We should log at info that the masking is being applied by workspace index
| bool forceDetIDs = getProperty("ForceInstrumentMasking"); | ||
| if (maskWs->getNumberHistograms() != WS->getNumberHistograms() || | ||
| forceDetIDs) { | ||
| extractMaskedWSDetIDs(detectorList, maskWs); |
There was a problem hiding this comment.
We should log at info that the masking is being applied by detector ids
|
Tested using the following script wsUnmasked = CreateSampleWorkspace()
wsMasked = CloneWorkspace(wsUnmasked)
def GeneratePattern(ws,noToGroup):
groupingPattern = []
for i in range(0,ws.getNumberHistograms(),noToGroup):
groupingPattern.append(str(i) + '+' + str(i+1))
groupingPatternString = ", ".join(groupingPattern)
return groupingPatternString
wsGrouped2 = GroupDetectors(wsUnmasked,GroupingPattern = GeneratePattern(wsUnmasked,2))
# Define an infinite cylinder with its axis parallel to the Z-axis
# and the radius of 0.04
shapeXML = \
"""
<infinite-cylinder id="A" >
<centre x="0" y="0" z="0" />
<axis x="0" y="0" z="1" />
<radius val="0.04" />
</infinite-cylinder>
"""
# Mask all detectors inside this cylinder
wsMasked = CloneWorkspace(wsUnmasked)
MaskDetectorsInShape( wsMasked, shapeXML )
wsGrouped2Masked = CloneWorkspace(wsGrouped2)
MaskDetectorsInShape( wsGrouped2Masked, shapeXML )
wsTestGrouped2MaskCopy = CloneWorkspace(wsUnmasked)
MaskDetectors(wsTestGrouped2MaskCopy,MaskedWorkspace=wsGrouped2Masked)
wsTestGroupedMaskCopy = CloneWorkspace(wsGrouped2)
MaskDetectors(wsTestGroupedMaskCopy,MaskedWorkspace=wsMasked)Then the instrument views of the workspaces were compared. Seems to work well |
|
@samueljackson92 There was a trivial conflict in the release notes. I've resolved it in the browser and I'll merge this assuming the builds pass. |
|
Failing Windows test is unrelated slice viewer failure. |
See the two scripts in the bug report for examples of how to reproduce the issue.
To test:
Two cases to check that didn't work before:
StartWorkspaceIndexandEndWorkspaceIndexFixes #14793 .
Reviewer
Please comment on the following (full description):
Code Review
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/gatekeeperswill take care of it.