-
Notifications
You must be signed in to change notification settings - Fork 75
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
fix subset histogram bug in plot options #2393
fix subset histogram bug in plot options #2393
Conversation
33ab93b
to
5377b9f
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2393 +/- ##
=========================================
+ Coverage 0 90.60% +90.60%
=========================================
Files 0 159 +159
Lines 0 18189 +18189
=========================================
+ Hits 0 16480 +16480
- Misses 0 1709 +1709
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to add a regression test into test_plot_options.test_stretch_histogram
if you have time, but otherwise looks good to me!
I am a little surprised that the existing if not self.stretch_function_sync.get('in_subscribed_states')
check higher up didn't exclude this scenario (since that is what excludes the UI from being shown... 🤔 ), but an extra check below to avoid crashing doesn't hurt anything.
Thanks for the find and fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that it works, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks
5377b9f
to
b5f4287
Compare
shall we merge this? |
ah, this was never tagged for backport, I'll try to kickstart it manually |
@meeseeksdev backport to v3.6.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
@meeseeksdev backport to v3.6.x |
…3-on-v3.6.x Backport PR #2393 on branch v3.6.x (fix subset histogram bug in plot options)
Description
When subsets are selected in Plot Options in Imviz/Cubeviz, the stretch histogram update raises an error. This PR adds a
return
to drop out of the stretch histogram update method when the selected layer is a subset, avoiding the error.This bug was discovered in #2387, splitting it out so it can be merged before 3.6.2.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.