Skip to content

feat: Add autoscale tail ignore via popup #147

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

Merged
merged 10 commits into from
Apr 4, 2025

Conversation

gselzer
Copy link
Collaborator

@gselzer gselzer commented Mar 1, 2025

This PR adds GUI functionality for autoscaling using ClimsPercentile, where the user can ignore the outlier samples of the dataset being displayed in autoscale:

Recording.2025-02-28.214155.mp4

I like the placement as a context menu because it means all autoscaling functionality appears within the same area of the canvas and because it normally does not take up any screen space.

Unfortunately, I do not know whether this design is possible with all our frontends, seeing as how I cannot figure out how to write a context menu in ipywidgets.

We might instead have to settle for adding this widget to the layouts added in #146...

@gselzer gselzer added the enhancement New feature or request label Mar 1, 2025
@gselzer gselzer requested a review from tlambert03 March 1, 2025 03:45
@gselzer gselzer self-assigned this Mar 1, 2025
Copy link

codecov bot commented Mar 1, 2025

Codecov Report

Attention: Patch coverage is 90.76923% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (a2e1442) to head (ab2af15).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ndv/views/_wx/_array_view.py 85.71% 5 Missing ⚠️
src/ndv/views/_qt/_array_view.py 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   84.76%   85.06%   +0.30%     
==========================================
  Files          45       45              
  Lines        4633     4694      +61     
==========================================
+ Hits         3927     3993      +66     
+ Misses        706      701       -5     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tlambert03
Copy link
Member

thanks @gselzer, this would indeed be great to have. Couple thoughts at this point:

  • I'm not sure a context menu is the correct way to show this here, it gives the strange appearance that the label is "actionable", and generally has the wrong look & feel. I think something like the QtPopup that I used to show an FPS selector back in v1 would be more appropriate (and generalizable: since I do need to get around to adding back those play buttons and will likely use that again).
    class QtPopup(QDialog):
    """A generic popup window."""
    def __init__(self, parent: QWidget | None = None) -> None:
    super().__init__(parent)
    self.setModal(False) # if False, then clicking anywhere else closes it
    self.setWindowFlags(Qt.WindowType.Popup | Qt.WindowType.FramelessWindowHint)
    self.frame = QFrame(self)
    layout = QVBoxLayout(self)
    layout.addWidget(self.frame)
    layout.setContentsMargins(0, 0, 0, 0)
    def show_above_mouse(self, *args: Any) -> None:
    """Show popup dialog above the mouse cursor position."""
    pos = QCursor().pos() # mouse position
    szhint = self.sizeHint()
    pos -= QPoint(szhint.width() // 2, szhint.height() + 14)
    self.move(pos)
    self.resize(self.sizeHint())
    self.show()
  • I would like to be able to set different percentiles for each end. (Does the mmstudio gui only have one number? I vaguely remember that being the case and not liking that... particularly with modern scmos cameras, you often want a different high number than low number)

@gselzer
Copy link
Collaborator Author

gselzer commented Mar 5, 2025

  • I think something like the QtPopup that I used to show an FPS selector back in v1 would be more appropriate (and generalizable: since I do need to get around to adding back those play buttons and will likely use that again).
  • I would like to be able to set different percentiles for each end.

Good ideas. Have updated in the latest commit:

image

If you're a fan of this now I'll investigate how/whether we can do similar things in wx (where I'm more confident) and jupyter (less confident)

@marktsuchida
Copy link

  • Does the mmstudio gui only have one number?

Yes, but I agree that separate settings are sometimes desirable.

I thought a bit about whether people would want the option of editing just one number, but then again, it doesn't make theoretical sense to use the same number for the read noise at the bottom and the shot noise at the top (for example). It would only be a heuristic convenience. Given that in many cases you'd only need a non-zero percentile for the upper limit (I think?), it seems fine (and simplest) to just have two fields.

@gselzer
Copy link
Collaborator Author

gselzer commented Apr 3, 2025

@tlambert03 I've been tinkering with a Jupyter version (aided by Claude) over the past few days, but I think I'd like to put that work on a separate branch, as I really want this functionality for qt right now. Any objections to punting on the jupyter functionality for now?

Still planning to investigate wx in the meantime

@gselzer gselzer force-pushed the autoscale-ignore-tails branch from 8a834a9 to 5418317 Compare April 3, 2025 15:48
@tlambert03
Copy link
Member

Any objections to punting on the jupyter functionality for now?

not at all. Similar to the play button, it’s fine with me if features hit different front ends at different times

@gselzer gselzer marked this pull request as ready for review April 3, 2025 21:18
@gselzer gselzer changed the title feat: Add autoscale tail ignore via context menu [WIP] feat: Add autoscale tail ignore via popup Apr 3, 2025
@gselzer
Copy link
Collaborator Author

gselzer commented Apr 3, 2025

@tlambert03 I'm pretty happy with these changes, if you'd like to take another look feel free, but can also just merge.

There's one bug I'm aware of but have not yet found a solution for - the tail edit controls for wx do not seem to be editable with keyboard. You can still edit it with the spin buttons (and that will be enough control for e.g. ignoring outliers) so I'm tempted to just merge and file an issue for this - thoughts?

@tlambert03
Copy link
Member

tlambert03 commented Apr 4, 2025

functionality is great. I find myself thinking/wondering about the terminology "tail" and "ignore" in "ignore upper/lower tail". I'm ok with that, but it's not strictly always "tail shaped"... some alternatives to consider (but if you prefer tail over all of these thats ok too:)

  • "clip" or "exclude" or "cutoff" instead of "ignore"
  • "percentile" or "darkest/brightest" instead of "tail"?

so:

Lower Percentile, Clip/Exclude Lower Percentile, Exclude/Clip Darkest

thoughts?

we could also have a checkbox that toggles all of it, so you can go back to min/max without changing the percentile values?

  • Clip Percentile
  • lower percentile: 0.00%
  • upper percentile: 100.0%

@tlambert03
Copy link
Member

tlambert03 commented Apr 4, 2025

the tail edit controls for wx do not seem to be editable with keyboard.

I can edit them, but it's awkward to "enter" ... (hitting enter doesn't work, I need to defocus) ... lemme try

edit: yeah: this is awkward... and I don't know how to fix it after trying a few things that i thought would have worked. (it's ok with me to leave this for later... it's not the only wx interface thing that is sub-optimal

@gselzer
Copy link
Collaborator Author

gselzer commented Apr 4, 2025

Exclude/Clip Darkest

Yeah, I like Exclude Darkest/Exclude Brightest! Will change momentarily.

we could also have a checkbox that toggles all of it, so you can go back to min/max without changing the percentile values?

Maybe - I was originally thinking tabs here, in case we want to add in controls for ClimsStdDev, etc. Could also just disable the text edits if the button is toggled off...

@tlambert03
Copy link
Member

Maybe - I was originally thinking tabs here, in case we want to add in controls for ClimsStdDev, etc. Could also just disable the text edits if the button is toggled off...

sure, let's consider in another PR

@gselzer
Copy link
Collaborator Author

gselzer commented Apr 4, 2025

Alright, I'll merge this after renaming the labels and create followup issues for (a) the Jupyter implementation and (b) the wx bug and (c) some toggle to retain settings when transitioning to manual mode.

@gselzer gselzer merged commit 5983fec into pyapp-kit:main Apr 4, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants