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

un-reverso the removeBackground flag #473

Merged
merged 5 commits into from
Nov 12, 2024
Merged

Conversation

rboston628
Copy link
Contributor

@rboston628 rboston628 commented Oct 15, 2024

Description of work

The flag to remove the background is called removeBackground, but is set backwards. If True it will NOT remove the background, and if False it WILL.

The easiest solution is to rename this dontRemoveBackground, and rename the associated UI elements.

However, the better solution is to have this work the way it was expected to.

Explanation of work

The flag was set to work with the meaning of "YES remove the background".

  • False --> do NOT remove background
  • True --> yes please DO remove background

It defaults to False

Also the checkbox to indicate this was made always visible. It was at first thought this was only wanted for advanced users in CIS mode, but @mguthriem has clarified this should be generally available.

The checkbox was changed to a toggle, for more unified UX.

While updating some of the DAOs, it is not necessary to involve the ConfigDict to set extra=forbid, so I removed that. A test was added of this.

To test

Dev testing

Run the script diffcal_pixel_diffraction_background_subtraction_script.py observe the BEFORE and AFTER workspaces to ensure the background is being removed.

Run the GUI with run 58882. Make sure

  • the toggle is visible when CIS mode is False
  • at the assess step, the workspace spectra have the background removed

CIS testing

After PR #498 is merged, you will be able to test the functionality when in CIS mode. Test as above. Note that this does not fix the bugs within RemoveEventBackground; it only addresses the meaning and visibility of the toggle.

Link to EWM item

EWM#7731

Following on from PR #449

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.

  • The checkbox is available for all users independent of CIS mode.
  • Checking the check box enables background removal.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.54%. Comparing base (8398c8e) to head (d947dc7).
Report is 5 commits behind head on next.

Files with missing lines Patch % Lines
src/snapred/backend/recipe/PixelDiffCalRecipe.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next     #473      +/-   ##
==========================================
+ Coverage   96.48%   96.54%   +0.06%     
==========================================
  Files          65       64       -1     
  Lines        4832     4836       +4     
==========================================
+ Hits         4662     4669       +7     
+ Misses        170      167       -3     

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

@rboston628 rboston628 marked this pull request as ready for review October 15, 2024 18:16
@walshmm
Copy link
Collaborator

walshmm commented Oct 25, 2024

Before check:
image
After check:
image

@walshmm
Copy link
Collaborator

walshmm commented Oct 25, 2024

not sure I notice a difference? is there a better run to highlight background removal?

@walshmm
Copy link
Collaborator

walshmm commented Oct 31, 2024

Running the script nets two (2) plots highlighting background subtraction:
image

  • The checkbox is available for all users independent of CIS mode.
    image
  • Checking the check box enables background removal.
    Please include full workflow parameters for testing, it makes prs more easily testable.
    I keep running into the "WARNING: More than 15.0% of pixels failed calibration. Please check your input data" which makes getting to the assess step impossible.
    Not sure if this is intended but choosing a new grouping and recalculating applies background removal it seems?
    image
    vs
    image

Bank/All received this error for some reason?
image

@mguthriem
Copy link
Collaborator

mguthriem commented Oct 31, 2024

These are issues that were already reported to Darsh and we've been trying to work out what's going wrong. The background fitting/subtraction is not working correctly.

Where we're at is that we concluded the background subtracted spectra have large amounts of noise for an unknown reason. That noise then corrupts the cross-correlation and the GetDetectorOffsets fails for > 15% of the peaks.

@rboston628
Copy link
Contributor Author

rboston628 commented Nov 1, 2024

@mguthriem While that is a defect, I don't it should block work on this defect. This work is just exposing the toggle to users and making sure it has the correct true/false relation. Nothing should have changed in whatever the background subtraction calculations are doing

walshmm
walshmm previously approved these changes Nov 5, 2024
Copy link
Collaborator

@walshmm walshmm left a comment

Choose a reason for hiding this comment

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

opened and confirmed that the remove background box is disabled and calibration completes successfully

@rboston628 rboston628 merged commit b721a42 into next Nov 12, 2024
7 of 8 checks passed
@rboston628 rboston628 deleted the patch-removeEventBackground branch November 12, 2024 20:33
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