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

Toggle shuttercount #2234

Merged
merged 7 commits into from
Jun 25, 2024
Merged

Toggle shuttercount #2234

merged 7 commits into from
Jun 25, 2024

Conversation

JackEAllen
Copy link
Collaborator

@JackEAllen JackEAllen commented Jun 21, 2024

Issue

Closes #2230

Description

Add a description of the changes made.

Modify Spectrum Viewer to add toggle interface for ShutterCount normalisation correction and warning icon when ShutterCount correction is toggled on, ShutterCount stacks are not loaded.

Testing

Describe the tests that were used to verify your changes.

  • Manually tested on both Windows and Ubuntu
  • Updated Unit tests and verified tests pass
  • Update GUI screenshot tests

Acceptance Criteria

How should the reviewer test your changes?

  • New Screenshot baselines for the spectrum viewer look okay
  • Load the following from a ToF dataset into Mantid Imaging:
    • Sample stack
    • Open beam stack
    • Spectra file
  • Open the spectrum viewer window
  • Resize the default ROI to something smaller and centralised on the sample you loaded in.
  • Verify that the ShutterCount checkbox is disabled and that you cannot enable ShutterCount correction.
  • Apply normalisation - verify that the ShutterCount checkbox only becomes active once a valid stack has been selected.
  • Apply ShutterCount correction and verify a warning Icon appears to warn that you "Need 2 selected stacks".
  • Un-check ShutterCount correction and verify that the error icon is removed from window
  • re-check ShutterCount correction and verify warning icon appears
  • From the main window, load ShutterCounts for sample stack then check that the warning has changed to ""Need 2 different stacks" and then load the ShutterCounts for the open beam stacks "File>Load ShutterCounts file for stack..."
  • Verify that the error icon is removed from the spectrum viewer window now that both ShutterCount stacks are loaded.
  • Notice how the spectrum scale changes accordingly when toggling ShutterCount correction on and off.

Documentation

How have you changed the documentation to reflect your changes? All changes should be noted in the appropriate file in docs/release_notes

docs/release_notes/next/feature-2230-ShutterCount_Correction_CheckBox

@JackEAllen JackEAllen self-assigned this Jun 21, 2024
@JackEAllen JackEAllen added Release 🚀 Issues related to creating a release Component: GUI Release essential Essential to the release - showstoppers. If not done the release might be delayed and removed Release 🚀 Issues related to creating a release labels Jun 24, 2024
@JackEAllen JackEAllen force-pushed the toggle_shuttercount branch 3 times, most recently from 6f31534 to bf6fd5b Compare June 24, 2024 15:03
@JackEAllen JackEAllen marked this pull request as ready for review June 24, 2024 16:51
@JackEAllen
Copy link
Collaborator Author

JackEAllen commented Jun 24, 2024

This PR changes the spectrum viewer GUI and therefore I have updated the screenshot test baselines

@coveralls
Copy link

Coverage Status

coverage: 73.171% (-0.1%) from 73.286%
when pulling ed3c260 on toggle_shuttercount
into 7af6d74 on main.

mantidimaging/gui/windows/main/view.py Outdated Show resolved Hide resolved
mantidimaging/gui/windows/spectrum_viewer/model.py Outdated Show resolved Hide resolved
mantidimaging/gui/windows/spectrum_viewer/model.py Outdated Show resolved Hide resolved
mantidimaging/gui/windows/spectrum_viewer/model.py Outdated Show resolved Hide resolved
@JackEAllen JackEAllen force-pushed the toggle_shuttercount branch from ed3c260 to 0260bf1 Compare June 25, 2024 10:03
@JackEAllen JackEAllen force-pushed the toggle_shuttercount branch from 0260bf1 to 73047cd Compare June 25, 2024 10:06
@coveralls
Copy link

Coverage Status

coverage: 73.157% (-0.1%) from 73.286%
when pulling 73047cd on toggle_shuttercount
into 7af6d74 on main.

@samtygier-stfc
Copy link
Collaborator

A small thing if its easy to fix.

  • With a dataset that does not have shutter counts
  • Open spectrum viewer, check "Normalise to open beam" and set norm stack
  • Check "ShutterCount Correction", the error should now be displayed
  • Change the Norm stack to the sample, this should the error for the norm to open beam, hides the shuttercount error, disables the shuttercount checkbox.
  • Change the Norm stack back to the open beam
  • Now the shutter count check box is enabled and checked, but not showing the error message.

The fix might be to uncheck the shuttercount check box when it is disabled.

@coveralls
Copy link

Coverage Status

coverage: 73.161% (-0.1%) from 73.286%
when pulling 916851f on toggle_shuttercount
into 7af6d74 on main.

@JackEAllen JackEAllen force-pushed the toggle_shuttercount branch from 916851f to 63e838c Compare June 25, 2024 12:43
@JackEAllen
Copy link
Collaborator Author

A small thing if its easy to fix.

  • With a dataset that does not have shutter counts
  • Open spectrum viewer, check "Normalise to open beam" and set norm stack
  • Check "ShutterCount Correction", the error should now be displayed
  • Change the Norm stack to the sample, this should the error for the norm to open beam, hides the shuttercount error, disables the shuttercount checkbox.
  • Change the Norm stack back to the open beam
  • Now the shutter count check box is enabled and checked, but not showing the error message.

The fix might be to uncheck the shuttercount check box when it is disabled.

Thanks for noticing this one, yeah, easy to fix, just uncheck if disabled within handle_button_enabled

@coveralls
Copy link

Coverage Status

coverage: 73.156% (-0.1%) from 73.286%
when pulling 63e838c on toggle_shuttercount
into 7af6d74 on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Looking good. I've gone through steps on the issue plus some extra playing around.

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Jun 25, 2024
@JackEAllen JackEAllen merged commit d407243 into main Jun 25, 2024
8 checks passed
@JackEAllen JackEAllen deleted the toggle_shuttercount branch June 25, 2024 14:18
@JackEAllen
Copy link
Collaborator Author

Looking good. I've gone through steps on the issue plus some extra playing around.

Thanks for the thorough review 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GUI Release essential Essential to the release - showstoppers. If not done the release might be delayed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spectrum Viewer: ShutterCount Front-end Toggle Switch
3 participants