-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Grocery keyset for increased safety calling Recipe #528
Conversation
Merge AFTER PR #526 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #528 +/- ##
==========================================
+ Coverage 95.80% 95.96% +0.16%
==========================================
Files 68 68
Lines 5105 5134 +29
==========================================
+ Hits 4891 4927 +36
+ Misses 214 207 -7 ☔ View full report in Codecov by Sentry. |
49b70cc
to
230af8c
Compare
230af8c
to
2507336
Compare
The recipe |
@@ -247,8 +247,8 @@ constants: | |||
highdSpacingCrop: 0.15 | |||
|
|||
RawVanadiumCorrection: | |||
numberOfSlices: 10 | |||
numberOfAnnuli: 10 | |||
numberOfSlices: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the tests, it's the difference between running in 1 second vs running in 1 minute
could you also add these AC and rationale to the story since this is more of a refactor? |
Defects can't have AC, but I added something to to description. |
performed some basic calibration, normalization, and reduction and didnt run into any regression issues. |
Description of work
Groceries are passed as a grocery, where the key roughly corresponds to a mantid
WorkspaceProperty
, and where the value is the name of a workspace.The
Recipe
ABC already had a methodmandatoryInputWorkspaces
, which was a set of workspaces which had to be given and had to exist in the ADS.However, for optional workspaces, it was possible a user could pass an option workspace with an incorrect key, and it would not set that workspace.
This was a problem in Defect 8554, where the
ReductionService
was passing a mask workspace with the key"combinedPixelMask"
, butReductionRecipe
was accessing the mask by the key"combinedMask"
, meaning the mask was not being set.This is fixed by also including a method
allGroceryKeys
, which ensures the groceries dictionary only has keys from that set.Explanation of work
Inside the
Recipe
ABC, there is a new methodallGroceryKeys
which returns the set of all possible keys that might be passed to this recipe. If the groceries contain a key not in this set, then an error is produced.Several workspaces inheriting from
Recipe
were refactored to implement this method. Some tests also had to be changed to accommodate the new behavior with workspace keys.The
ReductionRecipe
required additional work because of its_applyRecipe
method. This created a conflict, as theself.groceries
dictionary was being passed, which included all of the workspaces needed for reduction, but which are not needed for this the sub-recipes. This was reworked to not rely on theself.groceries
dictionary, and instead requires all input workspaces to be used, be passed as keyword arguments.@ekapadi noticed similar problems and also implemented some changes to this part of
ReductionRecipe
. Hopefully our changes are compatible.To test
Dev testing
Run all three workflows by hand to saving.
CIS testing
N/A
Link to EWM item
EWM#8554
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.
The simple fix to the defect was implemented in PR #526 . This PR implements the enhanced validation mentioned in the story.
Recipe
ABC implements better validation of groceriesReductionService
calls theReductionRecipe
with the correct key for the combined pixel mask