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

Ewm6381 fix pixel masks dropdown #506

Merged
merged 8 commits into from
Nov 27, 2024
Merged

Conversation

darshdinger
Copy link
Contributor

@darshdinger darshdinger commented Nov 21, 2024

Description of work

This PR addresses an issue where the "MaskWorkspace" dropdown in the pixel mask UI does not update correctly after reduction completion. This story has changed a little bit as instead of deleting all workspaces after Reduction is complete, we retain the outputs workspaces and the pixel mask workspaces. Therefore, we don't need to delete all the entries within the Pixel mask drop down but rather insure they are updated every time a run number is added or removed.

Explanation of work

The fix involves modifying the logic responsible for populating the pixel mask dropdown to ensure it fetches and displays an up-to-date list of workspaces. The following changes were made:

  1. Refreshing the Workspace list
  2. Triggering Updates at the Correct Workflow Stages
  3. Handling Deletions

To test

Dev testing

Start with a fresh stateless local directory and on the next branch. Open SNAPRed and run a Diffcal for run 58882 to completion.

Then go start the Reduction workflow for the same run 58882. Once you enter the run number, you will notice a pixel mask populate within the drop down. This is correct behavior as this pixel mask is stored within the Run's IPTS directory tree.

One portion of testing can be done now:

  1. Clear the run number list and notice the pixel mask is retained even though run 58882 is no longer within the run number list.
  2. Run reduction through with artificial norm and notice that we retain the reduced workspaces and the pixel mask.
  3. Hit the clear button on the main panel of SNAPRed to clear the workspaces and notice that the pixel mask is still retained within the drop down despite not being in the ADS.

Now switch over to this branch and re run testing steps above. You should see that the pixel mask drop down is updated correctly.

CIS testing

Not needed.

Link to EWM item

EWM # 6381

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.

  • Pixel mask drop resets as it should.

@darshdinger darshdinger marked this pull request as ready for review November 21, 2024 13:48
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.55%. Comparing base (1f0b521) to head (2963bb7).
Report is 1 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next     #506   +/-   ##
=======================================
  Coverage   96.55%   96.55%           
=======================================
  Files          66       66           
  Lines        4960     4960           
=======================================
  Hits         4789     4789           
  Misses        171      171           

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

rboston628
rboston628 previously approved these changes Nov 22, 2024
@rboston628 rboston628 self-requested a review November 22, 2024 03:13
@rboston628 rboston628 dismissed their stale review November 22, 2024 03:14

meant to approve request change, not entire PR

@rboston628
Copy link
Contributor

Test

Reproducing Original Error.

On next on analysis, I go to Reduction and enter run 58882. There is the pixel mask.
image

I delete that run, and it is still there.
image

I run Reduction on 58882 with this mask. When the workflow completes, it is still in the drop down, and the pixel mask workspace is in the ADS (this should NOT be happening, there is a call to delete these workspaces inside the workflow...).
image

Clear all workspaces, the pixel mask is still there.
image

Verifying the Fix

Moving to this branch, enter run 58882. Pixel mask appears in drop-down. Clear runs, pixel mask is gone from drop-down.

Run reduction on 58882 with the pixel mask. At the end it is still in the ADS even though it should not be. Clearing the runs clears the dropdown.
image

I can confirm the issue with the pixel mask drop down is fixed, but I'm not sure why the diffcal table and pixel mask are remaining...

@rboston628
Copy link
Contributor

On analysis, from this branch, ran Reduction on 58882 without loading any pixel mask. Reduction finishes, and the only workspaces remaining in the ADS list are the outputs.
image

Clear, then run again but with the mask. The pixel mask workspace and the diffcal table are still there and are not cleared.
image

This very likely is due to these workspaces being mistakenly placed in the self.outputs array.

@darshdinger
Copy link
Contributor Author

A new defect has been created to take care of the mask workspace retention within ADS as this work would be deemed out of scope for this particular defect. This defect only deals with the pixel mask drop down.

Here is the link to the new defect: Defect 8487

Copy link
Collaborator

@dlcaballero16 dlcaballero16 left a comment

Choose a reason for hiding this comment

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

Tested and dropdown resets correctly.

@darshdinger darshdinger merged commit 0720419 into next Nov 27, 2024
8 checks passed
@darshdinger darshdinger deleted the ewm6381-fix-pixel-masks-dropdown branch November 27, 2024 19:17
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