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

actually skip pixel calibration, using default calibration as startin… #486

Closed
wants to merge 4 commits into from

Conversation

walshmm
Copy link
Collaborator

@walshmm walshmm commented Oct 28, 2024

…g point.

Description of work

This pushes the skipPixelCalibration flag to just one place, and then actually sets the utilized place, ie on ingredients
Since pixel calibration was now actually being successfully skipped DiffractionCalibration ran into the issue of not having a starting diffcal table that pixel calibration would normally provide.

As per consultation of Malcolm, the default diffcal table is loaded from disk and used as the base in this case.
This has the secondary purpose of priming calibration for when we eventually add the ability to choose a starting calibration.

To test

Currently I kinda need help examining the data to confirm that it does what is expected.
I have confirmed codewise that pixel calibration is successfuly skipped when toggled, but the output workspace I was inspecting still kinda just looked the same.

I used the following in the diffcal flow.
59039
LA11b6

Link to EWM item

EWM#7617

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

The story is lacking explicit acceptance criteria, the title may suffice.

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.

  • acceptance criterion 1
  • acceptance criterion 2

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

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

Project coverage is 59.48%. Comparing base (bb8cf23) to head (bb7a20a).
Report is 7 commits behind head on next.

Files with missing lines Patch % Lines
src/snapred/backend/service/CalibrationService.py 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next     #486      +/-   ##
==========================================
- Coverage   59.51%   59.48%   -0.03%     
==========================================
  Files          64       64              
  Lines        4836     4840       +4     
==========================================
+ Hits         2878     2879       +1     
- Misses       1958     1961       +3     

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


🚨 Try these New Features:

@walshmm walshmm marked this pull request as ready for review October 28, 2024 20:22
@rboston628
Copy link
Contributor

rboston628 commented Oct 28, 2024

I'd strongly recommend not merging this right now, since the story I'm working on (7388 and the entire Epic 4161) is meant to completely refactor the entire diff cal process, including the pixel cal skip.

@rboston628
Copy link
Contributor

PR #488 went in, so this should be safe to work on again

@walshmm walshmm force-pushed the ewm7617_actually_skip_pixel_calibration branch from 7b4df8c to 5d1191c Compare November 6, 2024 18:02
@@ -8,4 +8,3 @@
class SimpleDiffCalRequest(BaseModel):
ingredients: DiffractionCalibrationIngredients
groceries: Dict[str, str]
skipPixelCalibration: bool
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 it'd actually be better to remove this from the ingredients instead. It is only used within the service layer

@walshmm walshmm force-pushed the ewm7617_actually_skip_pixel_calibration branch from 0d3dd5e to bb7a20a Compare November 13, 2024 20:29
@rboston628
Copy link
Contributor

Some of this was taken over in PR #497 , and so this PR might no longer be needed,

@rboston628 rboston628 mentioned this pull request Nov 20, 2024
6 tasks
@rboston628
Copy link
Contributor

Since merger of PR #497, running from next.

Open diffraction calibration. Run for 46680 with skip pixel calibration on. The workspace list is empty.
image

Continuing, it loads the default diffcal table and goes to tweak peak.
image

Continuing, it gets to the save screen. Saving completes successfully.
image

However, trying to run without skipping, hitting error, then running again with skipping, generates this error:
image

@rboston628
Copy link
Contributor

PR #507 fixed the remaining defect.

From next, run with 46680 and skip turned OFF. It pops up this error:
image

Then flip the toggle to ON, continue. Able to finish workflow:
image

@rboston628 rboston628 closed this Nov 21, 2024
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