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

Ewm3143 apply diffcal during norm workflow #469

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

walshmm
Copy link
Collaborator

@walshmm walshmm commented Oct 9, 2024

Description of work

This supplies the features requested in ewm3143:

  • The application of a diffraction calibration to the normaliztion data in the Normalization Workflow for user assessment
  • The logging of metadata on the workspace if it uses the default calibration in the Normalization Workflow

This also supplies features in the neighborhood of what it asked for:

  • The logging of metadata on the workspaces if diffraction calibration/normalization is missing in the Reduction Workflow

I have also refactored:

  • StrEnum to live in a seperate Enum module.
  • WorkspaceMetadata now uses StrEnum instead of literals.
  • Recipes now can supply an init method for their given Ingredients.

To test

NOTE: Use the View Sample Logs option when right clicking on a workspace to confirm it has the snapred metadata.

For reduction
Runnumber: 59039 

for calib:
sample: LA11b6
Grouping: Column
Peak Intensity Thresh: 0.01
Chi: 200
dmin: 1

for norm:
rnunumber: 58810
bgrunnumber: 58813

Attempt to run either [Normalization Worflow, Reduction Workflow] -> [without a state, with a default state, with a valid state]

Normalization

without a state

  1. should return a runtime error if you lack permission to init state
  2. should ask you to init state if you have write permission

with a default state

  1. Should prompt with a continue anyway informing the user that the output will be marked as potentially inaccurate by means of it using a default state.

with a valid state

  1. Continue as normal

Reduction

As it is in next except:

  • it actually loads the diffcal mask
  • the outputs should have appropriately marked metadata

Link to EWM item

EWM#3143

Verification

  • the author has read the EWM story and acceptance critera (Malcolm has additionally review and asked me to add AC)
  • 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.

Unfortunately there is not an explicity Acceptance Criteria outlined in the story, what I can infer from it are as follows
Malcolm has since reviewed the AC and made clarifications.

  • in the normalisation workflow, the raw vanadium data shall be treated exactly the same as the sample data being Reduced for examination.
  • in the normalisation workflow, If there is only a default diffcal (This workflow requires at least an initialized state), there should be a warning that the output will be labeled as using such default diffcal and proceed anyway prompt.
  • The outputs should have a log on them denoting the existence/status of diffcal/normalization.
  • in the normalisation workflow, the raw vanadium data shall be treated exactly the same as the sample data being Reduced for examination.
  • the difcal index is queried to find available difcal for the normalization run
  • if found the diffcal is applied to the raw
  • Any corresponding diffcal mask is also applied to the raw vanadium

@walshmm walshmm force-pushed the ewm3143_apply_diffcal_during_norm_workflow branch from 7683229 to 90a64d5 Compare October 10, 2024 18:31
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 40.98361% with 72 lines in your changes missing coverage. Please review.

Project coverage is 52.77%. Comparing base (523b781) to head (b420de1).
Report is 2 commits behind head on next.

Files with missing lines Patch % Lines
src/snapred/backend/service/ReductionService.py 12.00% 22 Missing ⚠️
src/snapred/backend/recipe/Recipe.py 39.13% 14 Missing ⚠️
...rc/snapred/backend/service/NormalizationService.py 54.83% 14 Missing ⚠️
src/snapred/backend/data/LocalDataService.py 38.46% 8 Missing ⚠️
...napred/backend/recipe/PreprocessReductionRecipe.py 16.66% 5 Missing ⚠️
src/snapred/backend/recipe/ReductionRecipe.py 0.00% 4 Missing ⚠️
src/snapred/backend/data/GroceryService.py 40.00% 3 Missing ⚠️
src/snapred/backend/data/DataFactoryService.py 50.00% 1 Missing ⚠️
...d/backend/recipe/ReductionGroupProcessingRecipe.py 75.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (523b781) and HEAD (b420de1). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (523b781) HEAD (b420de1)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             next     #469       +/-   ##
===========================================
- Coverage   96.39%   52.77%   -43.63%     
===========================================
  Files          65       66        +1     
  Lines        4740     4834       +94     
===========================================
- Hits         4569     2551     -2018     
- Misses        171     2283     +2112     

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

@walshmm walshmm marked this pull request as ready for review October 22, 2024 19:58
…as intended

just a couple line breaks

fix tests

up coverage

fix broken test

mark workspaces with metadata if no calibration/normalization is present

up test coverage

up timeout so integration test doesnt fail?

actually account for closing the new QMessageBox.warning from new alt flow

normalization workflow should initialize state and then warn the user its using default diffcal

revert integration_test.yml

fix integration tests, update state init and perms check for reduction

up test coverage

remove completed TODO

fix incorrectly scoped patch
@walshmm walshmm force-pushed the ewm3143_apply_diffcal_during_norm_workflow branch from 0a0eccb to 4e2852e Compare October 25, 2024 20:15
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.

1 participant