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

Ewm7121 refactor art norm #487

Merged
merged 13 commits into from
Oct 30, 2024
Merged

Ewm7121 refactor art norm #487

merged 13 commits into from
Oct 30, 2024

Conversation

darshdinger
Copy link
Contributor

@darshdinger darshdinger commented Oct 28, 2024

Description of work

The purpose for this PR is the implement the previously created artificial normalization within the Reduction workflow. 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:

  1. Artificial Normalization:
  • Added the ArtificialNormalizationRecipe to GenericRecipe that uses the CreateArtificialNormalizationAlgo to handle cases when normalization data is missing.
  • Automatically extracts parameters and queues the algorithm to create an artificial normalization workspace when needed.
  1. Workflow Updates:
  • The Reduction workflow can now dynamically adjust for missing calibration or normalization data, applying artificial normalization when necessary.
  • Updated the UI to prompt users for artificial normalization inputs and seamlessly continue the workflow.
  1. Error Handling and Logging:
  • Improved logging and error messages to guide users through situations where data is missing.
  • Enhanced exception handling to gracefully manage missing states or permissions.
  1. Backend Improvements:
  • Refactored the ReductionService to handle normalization and calibration more flexibly.
  • Expanded unit tests to cover new cases with artificial normalization.

To test

Insure all pytests pass. Please launch SNAPRed UI with a fresh (no states initialized) local copy of Calibration_next specified within application.yml. First run the Diffraction Calibration with the following inputs:

  • Run Number: 58882
  • Convergence Threshold: 0.1
  • Bins Across Peak Width: 10
  • Sample: Silicon_NIST_640D_001.json
  • Grouping File: Column

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 Continue button and notice the UI handling for no inputs:

image

Finally, without running a normalization, run the Reduction workflow with the following inputs:

  • Run Number: 58882

Continue the workflow to notice the following message pop-up:

image

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 the Artificial Normalization flow 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

  • 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.

  • 1) The user can input variables within an "artificial normalisation" step following a UI prompt informing the user that an artificial normalization is needed.
  • 2) The backend can calculate the arficifial normalisation function
  • 3) The backend applied the artificial normalisation function
  • 4) The output reduced data are available for all pixel groups (as per normal)
  • 5) The output data are labelled as having had artificial normalisation applied.

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.35%. Comparing base (09342de) to head (53d2f01).
Report is 1 commits behind head on next.

Files with missing lines Patch % Lines
src/snapred/backend/service/ReductionService.py 94.59% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next     #487       +/-   ##
===========================================
+ Coverage   55.65%   96.35%   +40.69%     
===========================================
  Files          64       64               
  Lines        4713     4744       +31     
===========================================
+ Hits         2623     4571     +1948     
+ Misses       2090      173     -1917     

☔ 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 October 28, 2024 21:24
@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

When attempting to proceed with neither normalization nor calibration and an uninitialized state I get an error
image
though this error I think gets fixed in my other pr https://github.com/neutrons/SNAPRed/pull/469/files#diff-8f698031b2852fad83d059775e7f13b373b64302000cb3f5dc92d1556c0f6ed0R90-R106
so I will init state elsewhere and proceed with further testing

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

Attempting to proceed with an a freshly initialized state nets the following error
image
this is the current state of my data folder
image
I am able to load default calibration with my pr here https://github.com/neutrons/SNAPRed/pull/486/files#diff-494c3d2dbf59e6e214cb384bd29120e6bc67085459feb33eff42d2e32caeb3dcR169-R175
so I am unsure why this fails.

I will perform a calibration and proceed again, but the above needs fixing

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

I forgot to note in my last comment that I did get an error message specifying that diffcal and norm was missing but it said the data would not be normalized, is that correct? I though we would apply an artificial norm.

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

Even with diffcal created it seems to be erroring out?
image
Why are you specifying column here? https://github.com/neutrons/SNAPRed/pull/487/files#diff-8f698031b2852fad83d059775e7f13b373b64302000cb3f5dc92d1556c0f6ed0R484-R485
I tried a grouping of All and its failing

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

recalibrating specifically with a column grouped calibration allows me to proceed
image
However these true false fields are unlabeled, please give them some.

Reduction then proceeds and completes successfully

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

Reduction with a proper calibration and normalization proceeds and completes
image
Successfully skipping artificial norm and informing the user that it is not necessary.

Comment on lines 480 to 487
calVersion = None
calVersion = self.dataFactoryService.getThisOrLatestCalibrationVersion(request.runNumber, request.useLiteMode)
calRecord = self.dataFactoryService.getCalibrationRecord(request.runNumber, request.useLiteMode, calVersion)
filePath = self.dataFactoryService.getCalibrationDataPath(request.runNumber, request.useLiteMode, calVersion)
diffCalOutput = calRecord.workspaces[wngt.DIFFCAL_OUTPUT][0]
diffcalOutputFilePath = str(filePath) + "/" + str(diffCalOutput) + ".nxs.h5"

groceries = self.groceryService.fetchWorkspace(diffcalOutputFilePath, "diffractionWorkspace")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you move this logic to a method on groceryService instead? I can imagine it would be very useful elsewhere to be able to read a calibration's run data with a simple (runnumber, litemode, calversion).

It would also be more in line with the rest of the architecture.

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

fresh state displays this:
image
default state lacks data:
image

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

All Grouping Calibration works now
image
true false fields are also labeled now

@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

proceeding anyway despite there being no calibration now nets an error explaining that this feature has yet to be implemented yet due to lack peaks available from the default calibration.
image

@darshdinger darshdinger enabled auto-merge (squash) October 30, 2024 19:38
@walshmm
Copy link
Collaborator

walshmm commented Oct 30, 2024

  • 1) The user can input variables within an "artificial normalisation" step following a UI prompt informing the user that an artificial normalization is needed.
    image

  • 2) The backend can calculate the arficifial normalisation function
    image
    image

  • 3) The backend applied the artificial normalisation function
    I think that looks right.
    image

  • 4) The output reduced data are available for all pixel groups (as per normal)
    image

  • 5) The output data are labelled as having had artificial normalisation applied.
    this is actually taken care of in a different pr

@darshdinger darshdinger merged commit 66af7ef into next Oct 30, 2024
8 checks passed
@darshdinger darshdinger deleted the ewm7121-refactor-art-norm branch October 30, 2024 19:49
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.

2 participants