Implement Create Artificial Norm Algo within Reduction Workflow #463
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #463 +/- ##
==========================================
+ Coverage 96.53% 96.56% +0.02%
==========================================
Files 63 63
Lines 4503 4541 +38
==========================================
+ Hits 4347 4385 +38
Misses 156 156 ☔ View full report in Codecov by Sentry. |
| class ArtificialNormResponse(BaseModel): | ||
| diffractionWorkspace: WorkspaceName | ||
|
|
||
| model_config = ConfigDict( |
There was a problem hiding this comment.
We should probably pick one or the other format for specifying model configs.
I like the class based one here but I think we use the above more often in the code
| tb = exception.__traceback__ | ||
| def __init__(self, exception): | ||
| # Handle both string and Exception types for 'exception' | ||
| if isinstance(exception, Exception): |
There was a problem hiding this comment.
I'm not sure we should be expanding exceptions type to be both Exception and str, I dont think I see the usecase in this pr? I just see this used in a test file.
There was a problem hiding this comment.
It is used in the test.
There was a problem hiding this comment.
This has been reverted
| if request.artificialNormalization is not None: | ||
| continueFlags = ContinueWarning.Type.UNSET | ||
|
|
||
| # check that the user has write permissions to the save directory | ||
| if not self.checkWritePermissions(request.runNumber): | ||
| continueFlags |= ContinueWarning.Type.NO_WRITE_PERMISSIONS | ||
|
|
||
| # remove any continue flags that are present in the request by xor-ing with the flags | ||
| if request.continueFlags: | ||
| continueFlags = continueFlags ^ (request.continueFlags & continueFlags) | ||
|
|
||
| if continueFlags: | ||
| raise ContinueWarning( | ||
| f"<p>Remeber, you don't have permissions to write to " | ||
| f"<br><b>{self.getSavePath(request.runNumber)}</b>,<br>" | ||
| + "but you can still save using the workbench tools.</p>" | ||
| + "<p>Would you like to continue anyway?</p>", | ||
| continueFlags, | ||
| ) | ||
| else: | ||
| continueFlags = ContinueWarning.Type.UNSET | ||
| warningMessages = [] | ||
|
|
||
| # check if a normalization is present | ||
| if not self.dataFactoryService.normalizationExists(request.runNumber, request.useLiteMode): | ||
| continueFlags |= ContinueWarning.Type.MISSING_NORMALIZATION | ||
| warningMessages.append("Normalization is missing, continuing with artificial normalization step.") | ||
| # check if a diffraction calibration is present | ||
| if not self.dataFactoryService.calibrationExists(request.runNumber, request.useLiteMode): | ||
| continueFlags |= ContinueWarning.Type.MISSING_DIFFRACTION_CALIBRATION | ||
| warningMessages.append("Diffraction calibration is missing, continuing with uncalibrated mode.") | ||
|
|
||
| # remove any continue flags that are present in the request by xor-ing with the flags | ||
| if request.continueFlags: | ||
| continueFlags = continueFlags ^ (request.continueFlags & continueFlags) | ||
|
|
||
| if continueFlags: | ||
| detailedMessage = "\n".join(warningMessages) | ||
| raise ContinueWarning( | ||
| f"The reduction cannot proceed due to missing data:\n{detailedMessage}\n", continueFlags | ||
| ) | ||
|
|
||
| # ... ensure separate continue warnings ... | ||
| continueFlags = ContinueWarning.Type.UNSET | ||
| # ... ensure separate continue warnings ... | ||
| continueFlags = ContinueWarning.Type.UNSET | ||
|
|
||
| # check that the user has write permissions to the save directory | ||
| if not self.checkWritePermissions(request.runNumber): | ||
| continueFlags |= ContinueWarning.Type.NO_WRITE_PERMISSIONS | ||
| # check that the user has write permissions to the save directory | ||
| if not self.checkWritePermissions(request.runNumber): | ||
| continueFlags |= ContinueWarning.Type.NO_WRITE_PERMISSIONS | ||
|
|
||
| # remove any continue flags that are present in the request by xor-ing with the flags | ||
| if request.continueFlags: | ||
| continueFlags = continueFlags ^ (request.continueFlags & continueFlags) | ||
| # remove any continue flags that are present in the request by xor-ing with the flags | ||
| if request.continueFlags: | ||
| continueFlags = continueFlags ^ (request.continueFlags & continueFlags) | ||
|
|
||
| if continueFlags: | ||
| raise ContinueWarning( | ||
| f"<p>It looks like you don't have permissions to write to " | ||
| f"<br><b>{self.getSavePath(request.runNumber)}</b>,<br>" | ||
| + "but you can still save using the workbench tools.</p>" | ||
| + "<p>Would you like to continue anyway?</p>", | ||
| continueFlags, | ||
| ) | ||
| if continueFlags: | ||
| raise ContinueWarning( | ||
| f"<p>It looks like you don't have permissions to write to " | ||
| f"<br><b>{self.getSavePath(request.runNumber)}</b>,<br>" | ||
| + "but you can still save using the workbench tools.</p>" | ||
| + "<p>Would you like to continue anyway?</p>", | ||
| continueFlags, | ||
| ) |
There was a problem hiding this comment.
This and this are duplicate code. Please break this method out into smaller sub methods to reduce its complexity and the amount of nesting.
i.e. this should be its own method, and this should be another method, and those two should be substituted into the above method like so(I reorged it so we didnt need to check write perms twice):
if request.artificialNormalization is None:
self._validateCalibrationAndNormalizationExists(request)
self._validateWritePermissions(request)
| groceries = self.fetchReductionGroceries(request) | ||
| if isinstance(groceries, ArtificialNormResponse): | ||
| return groceries |
There was a problem hiding this comment.
Again we shouldnt be expanding type like this. I'll probably have another comment when I get to prepReductionIngredients.
| calVersion = None | ||
| normVersion = None | ||
| calVersion = self.dataFactoryService.getThisOrLatestCalibrationVersion( | ||
| request.runNumber, request.useLiteMode | ||
| ) | ||
| self.groceryClerk.name("diffcalWorkspace").diffcal_table(request.runNumber, calVersion).useLiteMode( | ||
| request.useLiteMode | ||
| ).add() |
There was a problem hiding this comment.
this is very similar code to the above, I dont think normVersion is used in either of the highlighted snippets, so we can extract this duplicate code out and above the if/else statement.
| self._artificialNormalizationView.updateWorkspaces(diffractionWorkspace, artificialNormWorkspace) | ||
| else: | ||
| print(f"Error: Workspaces not found in the response: {response.data}") | ||
| except Exception as e: # noqa: BLE001 |
There was a problem hiding this comment.
Please dont swallow errors like this, let it fail. Its much harder to track down without a stacktrace, or rewrap it with a human readable error message and rethrow.
| @@ -149,7 +162,7 @@ def _triggerReduction(self, workflowPresenter): | |||
| ) | |||
|
|
|||
| response = self.request(path="reduction/", payload=request_) | |||
| if response.code == ResponseCode.OK: | |||
| if isinstance(response.data, ReductionResponse): | |||
There was a problem hiding this comment.
If I am understanding this correctly, you:
- Attempt reduction
- If it turns out you need an artificial normalization(As determined by the response type containing one workspace name) then you
- Create the base Artificial Normalization workspace for the next step
- Allow the user to tweak the AN
- Proceed with Reduction
I think it would be more straight forward if you didnt shoehorn the check into the same method as reduction.
- User inputs values in first view -> method to check if there needs to be an AN, create one if necessary, update/skip AN view
- No AN Required? The AN view is either skipped entirely or greyed out with a message letting the user know no AN was required.
- Yes AN Required? The AN is initialized and the user tweaks it in the AN view
- The AN Tweak View is utilized or skipped -> Proceed with Reduction
- Save.
This way you only ever call reduction in one spot and you have a linear non branching chain of events. It should reduce the complexity of the code.
There was a problem hiding this comment.
(You would also be rid of the isinstance branching, and can utilize typical response procedure)
There was a problem hiding this comment.
According to the acceptance criteria of the parent story
The user can select "artificial normalisation" via a UI prompt and submit the two parameters above.
Unfortunately Im not sure it was supposed to be a step in reduction 😔, please check with Malcolm to see if this is acceptable or if you need to move it.
| # Update the view with new workspaces | ||
| self._artificialNormalizationView.updateWorkspaces(diffractionWorkspace, artificialNormWorkspace) | ||
|
|
||
| except Exception as e: # noqa: BLE001 |
There was a problem hiding this comment.
Same as above, dont swallow all exceptions arbitrarily, this will be a massive headache to someone.
|
Please reference the base Story 6833 for the full scope of Acceptance Criteria |
| # Note that the run number is deliberately not deleted from the run numbers list. | ||
| # Almost certainly it should be moved to a "completed run numbers" list. | ||
|
|
||
| # SPECIAL FOR THE REDUCTION WORKFLOW: clear everything _except_ the output workspaces | ||
| # _before_ transitioning to the "save" panel. | ||
| # TODO: make '_clearWorkspaces' a public method (i.e make this combination a special `cleanup` method). | ||
| self._clearWorkspaces(exclude=self.outputs, clearCachedWorkspaces=True) | ||
| workflowPresenter.advanceWorkflow() |
There was a problem hiding this comment.
Disable ArtificialNormView here as well.
Description of work
The purpose for this PR is the implement the previously created artificial normalization within the
Reductionworkflow. The changes ensure the workflow can handle missing normalization data by applying artificial normalization and introduce enhancements to the user interface, logging, and data handling processes.Explanation of work
Key updates include:
ArtificialNormalizationRecipetoGenericRecipethat uses theCreateArtificialNormalizationAlgoto handle cases when normalization data is missing.Reductionworkflow can now dynamically adjust for missing calibration or normalization data, applying artificial normalization when necessary.ReductionServiceto handle normalization and calibration more flexibly.To test
Insure all pytests pass. Please launch SNAPRed UI with a fresh (no states initialized) local copy of
Calibration_nextspecified withinapplication.yml.Firstrun the Diffraction Calibration with the following inputs:Create a new state when prompted. Then continue on through workflow through successful completion.
Now moving onto the Reduction tab, notice there is a new tab called "Artificial Normalization". Press the
Continuebutton and notice the UI handling for no inputs:Finally,
withoutrunning a normalization, run theReductionworkflow with the following inputs:Continue the workflow to notice the following message pop-up:
Continue Past the possible warning for permissions check but clicking
Yes. Find that the workflow completes successfully.Finally, re-run this but with a normalization this time and see that theArtificial Normalizationflow is not used in this case.Dev testing
Insure all pytests pass
CIS testing
See above.
Link to EWM item
EWM # 7123
&
EWM # 7121
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.