Ewm3645 mask workspace diffcal #658
Conversation
fix import path apparently the auto import logic has changed for the worse fix fetch diffcal groceries unit test odd update to try and get github to kick off ci add test for the combining of masks for diffcal actually pass the maskworkspace to be used by the backend update tests to accept maskworkspaces from different states as valid fix integration tests added a test to confirm resident masks are filtered if not compatible change exception type to something not pydantic up the last couple lines of coverage update precommit, remove commented out code fix mock method definition
…workspace_diffcal
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #658 +/- ##
==========================================
- Coverage 96.34% 96.27% -0.07%
==========================================
Files 77 77
Lines 7010 7043 +33
==========================================
+ Hits 6754 6781 +27
- Misses 256 262 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ekapadi
left a comment
ekapadi
left a comment
There was a problem hiding this comment.
I got through comments for everything but the tests. I'll go through those next. Most of these comments, with a few exceptions, are just nit-picking. However, they'll probably show a few things that you could help explain!
src/snapred/backend/recipe/algorithm/FitMultiplePeaksAlgorithm.py
Outdated
Show resolved
Hide resolved
| for index in range(self.count()): | ||
| item = self.model().item(index) | ||
| if item.text() == value: | ||
| item.setCheckState(Qt.Checked) |
There was a problem hiding this comment.
I was wondering if we should disable .. setCheckState .. enable here? Just double checking...
There was a problem hiding this comment.
Im not totally sure of the nuance. Just so users cant mess with it while its updating?
There was a problem hiding this comment.
No, it's so you don't send multiple widget-changed messages due to the programmatic setCheckState. I did not consider the entire chain, but my understanding is generally we would want: <user click event> -> "disable" -> <programmatic widget changes> -> "enable". Otherwise we see: <user click event> -> <programmatic widget changes> -> ... <additional events triggered by programmatic changes> ...
There was a problem hiding this comment.
I dont think this programmatic change triggers another event that we depend on? I can disable it in defense of in case we do.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
49e6036 to
6499fcb
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
1bef35a to
b720b8c
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
94b6307 to
a87bd8f
Compare
for more information, see https://pre-commit.ci
ekapadi
left a comment
ekapadi
left a comment
There was a problem hiding this comment.
I had some issues with the way combinePixelMasks creates and uses its <output workspace>. I think I'd prefer that it would explicitly create it, not depending on any "it's the first workspace in the incoming list" type of behavior. The problem is that, in workbench, the user could be working on a bunch of different things at the same time. It is not OK that one of their masks suddenly dissappeared and was re-purposed for SNAPRed's purposes. :)
|
|
||
| def combinePixelMasks(self, outputMaskWsName: WorkspaceName, masks2Combine: List[WorkspaceName]): | ||
| startingIndex = 0 | ||
| if len(masks2Combine) <= 0: |
There was a problem hiding this comment.
Pythonically, this would be if not masks2Combine:, and if it were < 0 something would be really messed up with len. :)
| raise ValueError("Lists of masks to combine is empty") | ||
|
|
||
| if not self.mantidSnapper.mtd.doesExist(outputMaskWsName): | ||
| self.renameWorkspace(masks2Combine[0], outputMaskWsName) |
There was a problem hiding this comment.
No. I don't think we can assume its OK to overwrite the input mask in this manner! Here we can assume that we "own" the outputMaskWsName, but we need to leave the input workspaces alone. SO, I think that means in most cases you need to use fetchCompatiblePixelMask to create the output mask workspace, like before.
There was a problem hiding this comment.
I would like to avoid using fetchCompatiblePixelMask inside combinePixelMasks due to it requiring the addition of runnumber and useLiteMode.
I can probably just remove this case and require that outputMaskWs exists. Current consumers of this method already call fetchCompatiblePixelMask to generate outputMaskWs beforehand.
| ) | ||
| # purge empty string, diffcalmask comes back as one if it doesnt exist | ||
| allMasks = [m for m in allMasks if m] | ||
| allMasks.append(self.groceryService.fetchCompatiblePixelMask(combinedMask, runNumber, useLiteMode)) |
There was a problem hiding this comment.
As in the comment at GroceryService: I'd prefer if it creates a new output workspace each time. The idea of combining to the first workspace in the list is "interesting", but it's not idiomatic.
…of combinePixelMasks already exist
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
4026ef5 to
02fdbfe
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
52d1195 to
6fc6cf3
Compare
for more information, see https://pre-commit.ci
Description of work
This pr enables users of snapred to supply a user generated pixel mask to be used as part of diffraction calibration.
Explanation of work
A lot of the work in this pr was around all the edge cases in the sub-algorithms that didnt account for fully masked groups.
The other bits of work involved orchestrating the mask, and combining it with the potentially existing diffraction calibration mask.
I also refactored the diffcal section of the happy path tests to be much more modular.
This way I was able to include an additional test with the same sequence of events, except with the addition of testing for user masks.
A user mask influences the following steps of the workflow, so it necessitated a whole new run.
To test
Observe what the new integration test does.
If you would like to test it manually, you will need to load your target input workspace, or one with the same instrument as it.
Then mask out a whole group, extract the mask, and name it
MaskWorkspace_2.(I think I need to write a defect, as I encountered
MaskWorkspace_1's not properly being sent along, though that may be my fault?)Now run diffcal with this mask selected, the tweak peaks view should have at least one less graph if you masked out a group.
Run to completion.
Link to EWM item
EWM#3645
Verification
Acceptance Criteria
This list is for ease of reference, and does not replace reading the EWM story as part of the review. Verify this list matches the EWM story before reviewing.