Skip to content
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

Ewm7782 diffcal residual addtion #493

Merged
merged 11 commits into from
Nov 14, 2024
Merged

Conversation

darshdinger
Copy link
Contributor

@darshdinger darshdinger commented Nov 7, 2024

Description of work

This work adds functionality to calculate and display a residual plot in the "peek peak" view of the Diffraction Calibration (DIFCAL) workflow in the SNAPRed application. This residual plot aids in assessing the quality of Bragg peak fitting by displaying the difference between calculated and observed data in specific regions. The addition of the residual helps users quickly gauge fit accuracy, making it particularly useful in analyzing diffraction-focused data.

Explanation of work

To achieve this, several changes and new methods were introduced:

  1. Calculation of the Residual in DIFCAL:
  • The residual is calculated as the difference between two workspaces: dsp_column_f-dc_{run} (observed data) and fit_peak_diag_{run}_2_pre_fitted (calculated Bragg peak fit). Since the calculated workspace contains zero values outside peak regions, these were replaced with NaN values before performing the subtraction. This ensures that the residual plot only displays values where calculations are meaningful, improving interpretability and avoiding misleading zero-based residuals.
  1. Modification of the updateGraphs Method:
  • The updateGraphs method in the frontend "peek peak" view was modified to include the residual plot for each pixel subgroup. The residual is displayed as a green line with increased thickness for better visibility. This line plot uses a different color to distinguish it from the observed data and fit, adhering to design specifications.
  1. Design Considerations:
  • The residual is only displayed in regions where data and calculations are available. In DIFCAL, this limits the residual plot to the vicinity of Bragg peaks, while in NORMCAL, the residual spans the full spectrum. This differentiation was handled by selectively applying the NaN replacement technique for DIFCAL and leaving the full array intact for NORMCAL.
  • The peek peak view now consistently displays observed data, the calculated fit, and the residual in each panel, with color and line style distinctions to help users easily differentiate between them.
  1. Implementation Choices:
  • Zero Replacement with NaN: In the fit_peak_diag_{run}_2_pre_fitted workspace, zeros were replaced with NaN to avoid misleading zero-based residuals outside peak regions. This change leverages ReplaceSpecialValues, an algorithm that allows setting zero values to NaN with a threshold.
  • Residual Plot Styling: The residual line is rendered with a thicker, green line to ensure visibility, especially when displayed over dense data or calculated fit lines. This choice was made to meet the requirement for a distinct visual separation from other plot elements.
  • Flexible For-Loop Implementation: To handle non-sequential spectrum IDs in the DIFCAL data, the updateGraphs method includes logic to iterate through each spectrum by its actual SpectrumNumber, ensuring that data are consistently processed regardless of ID ordering.

To test

Dev testing

Please first insure all pytests pass.

To test, follow the following directions:

  1. Modify application.yml according to point to your local Calibration directory
  2. Make sure that you don't have any states initialized
  3. Start SNAPRed and run diffcal workflow with the following inputs:
  • Run Number: 58882
  • Convergence Threshold: 0.1
  • Bins Across Peak Width: 10
  • Sample: Silicone_NIST_001.json
  • Grouping File: Column or Any

Run diffcal worklfow, stop at Tweak Peak Peek to inspect residual present within the graphs shown. Please test recalculate and other features to insure full functionality is retained.

CIS testing

Please follow the steps above.

Link to EWM item

EWM # 7782

Verification

  • the author has read the EWM story and acceptance critera
  • the reviewer has read the EWM story and acceptance criteria
  • the reviewer certifies the acceptance criteria below reflect the criteria in EWM

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.

  • Residual Calculation should be present within the tweek peek peak view within the DIffcal Workflow
  • The DIffcal Workflow should be fully functional.

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.53%. Comparing base (bb8cf23) to head (14fa4c2).
Report is 1 commits behind head on next.

Files with missing lines Patch % Lines
...d/backend/recipe/CalculateDiffCalResidualRecipe.py 95.31% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next     #493       +/-   ##
===========================================
+ Coverage   59.51%   96.53%   +37.02%     
===========================================
  Files          64       65        +1     
  Lines        4836     4912       +76     
===========================================
+ Hits         2878     4742     +1864     
+ Misses       1958      170     -1788     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@darshdinger darshdinger marked this pull request as ready for review November 8, 2024 14:08
@rboston628
Copy link
Contributor

rboston628 commented Nov 11, 2024

The residual graph needs to update when the fit peaks do
before
image
after
image

@mguthriem
Copy link
Collaborator

For what it's worth, I've just tested this and think it's a very nice implementation. The residuals update upon recalculation as requested by Reece and continuing on from Peek Peak the workflow executes correctly.

I happened to use this to compare the Lorentzian and Gaussian peak shapes (for a silicon run) and the residual very clearly shows that the latter is much better. This is a nice feature!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be turned into a recipe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -356,7 +361,17 @@ def _renewFitPeaks(self, peakFunction):
detectorPeaks=self.ingredients.groupedPeakLists,
peakFunction=peakFunction,
)
return self.request(path="calibration/fitpeaks", payload=payload.json())
response = self.request(path="calibration/fitpeaks", payload=payload.json())
self._calculateResidual()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this call should occur in L294 and L301 instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 1155 to 1180
@mock.patch("snapred.backend.service.CalibrationService.CalculateResidualDiffCalRecipe")
def test_calculateResidual(self, MockCalculateResidualDiffCalRecipe):
# Arrange: set up mock request and mock recipe
mockRequest = mock.Mock(
spec=CalculateResidualRequest,
inputWorkspace="inputWS",
outputWorkspace="outputWS",
fitPeaksDiagnostic="fitPeaksDiagWS",
)

# Mock the recipe execution result
mockRecipeInstance = MockCalculateResidualDiffCalRecipe.return_value
mockRecipeInstance.executeRecipe.return_value = "expected_result"

# Act: call calculateResidual with the mock request
result = self.instance.calculateResidual(mockRequest)

# Assert: check that the recipe was called with correct parameters
MockCalculateResidualDiffCalRecipe.assert_called_once_with()
mockRecipeInstance.executeRecipe.assert_called_once_with(
InputWorkspace=mockRequest.inputWorkspace,
OutputWorkspace=mockRequest.outputWorkspace,
FitPeaksDiagnosticWorkSpace=mockRequest.fitPeaksDiagnostic,
)
# Assert that the result is what the recipe returns
self.assertEqual(result, "expected_result") # noqa: PT009
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been trying to transfer tests to use mock.sentinel. I think it should be used here instead of "inputWS", "outputWS", "fitPeaksDiagWS", and "expected_result".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to implement this, but couldn't get it to work properly.

Copy link
Contributor

@rboston628 rboston628 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run once, see the residual:
image

Update parameters, new residual:
image

@darshdinger darshdinger merged commit 74da998 into next Nov 14, 2024
8 checks passed
@darshdinger darshdinger deleted the ewm7782-diffcal-residual-addtion branch November 14, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants