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

Add polygon selection #371

Merged
merged 7 commits into from
Jul 14, 2023
Merged

Conversation

jfoster17
Copy link
Member

Pull Request Template

Description

This adds a polygon/lasso-type selection tool to the image and scatter viewers in glue-jupyter using the MouseInteraction class provided in bqplot_image_gl. Creating subsets from arbitrary regions is a top priority for Alyssa before we show an alpha version of glue-jupyter-lab to the world.

For similarity with other selection tools it would have been nice to get the data back from the LassoSelector (as in this stalled PR), but getting that finished and into a released version of bqplot was probably going to take too long.

I hope that the new testing infrastructure can verify that adding a new default tool to the basic viewers does not mess things up for jdaviz.

@jfoster17
Copy link
Member Author

Tagging @astrofrog and @maartenbreddels for their awareness here

@jfoster17
Copy link
Member Author

Not sure what the CircleCI error here means...

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Aside from a small comment about the icon below, I couldn't get this to work - for some reason the drag events are just not triggered. If I add 'click' to the events to listen to, then I can check that on_msg gets called when I click, but nothing happens when I click and drag. I'm using MacOS X and I see this on both Safari and Google Chrome.

glue_jupyter/bqplot/common/tools.py Outdated Show resolved Hide resolved
@astrofrog
Copy link
Member

The CircleCI error should be fixed by #372

@astrofrog
Copy link
Member

I just tried this out in Jupyter lab and it works nicely! One small difference compared to other tools is that when the selection is being drawn, it is bright yellow while drawing is active, whereas for other selection tools it is a translucent grey. Would it be possible to make this new tool match the behavior of the existing ones?

@jfoster17
Copy link
Member Author

I just tried this out in Jupyter lab and it works nicely! One small difference compared to other tools is that when the selection is being drawn, it is bright yellow while drawing is active, whereas for other selection tools it is a translucent grey. Would it be possible to make this new tool match the behavior of the existing ones?

Done.

@jfoster17
Copy link
Member Author

Do you get any error when trying to use this polygon tool in Notebook? I get:

[E 11:07:22.922 NotebookApp] Uncaught exception, closing connection.
    Traceback (most recent call last):
      File "/Users/jfoster/mambaforge/envs/bqplot-polygon-selection/lib/python3.11/site-packages/tornado/iostream.py", line 695, in _handle_events
        self._handle_write()
      File "/Users/jfoster/mambaforge/envs/bqplot-polygon-selection/lib/python3.11/site-packages/tornado/iostream.py", line 965, in _handle_write
        self._write_buffer.advance(num_bytes)
      File "/Users/jfoster/mambaforge/envs/bqplot-polygon-selection/lib/python3.11/site-packages/tornado/iostream.py", line 182, in advance
        assert 0 < size <= self._size
               ^^^^^^^^^^^^^^^^^^^^^^
    AssertionError
Exception in callback None()

which looks suspiciously similar to the iostream messages reported here.

@astrofrog
Copy link
Member

@jfoster17 - when I use notebook I don't see any errors - either in the browser console or in the terminal where I started the notebook from. Clicking and dragging just doesn't do anything.

@Carifio24
Copy link
Member

I'm seeing the same thing - works great in lab, but no errors and no functionality in the notebook. I found something that, at least for me, makes it work in both environments, but maybe @astrofrog has a bit more insight into why this would be the case.

So I think the issue might be the MouseInteraction that's set as the tool's interact member (maybe it's related to the comment here?). As it is now, for me all drag events in the notebook are completely silenced with this tool active - it seems that this interaction is eating them up somehow. But if I ditch this interaction (set self.interact = None), this works fine for me in both the notebook and lab. (Why this isn't an issue in lab, I have no clue).

@jfoster17
Copy link
Member Author

Fantastic! Thanks for this breakthrough Jon. Now there are just a few more things to clean up...

@jfoster17 jfoster17 force-pushed the add-polygon-selection branch from 0d29c54 to b12d66a Compare July 14, 2023 00:42
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 29.82% and project coverage change: -0.67 ⚠️

Comparison is base (eff2a12) 87.32% compared to head (b12d66a) 86.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
- Coverage   87.32%   86.65%   -0.67%     
==========================================
  Files          89       89              
  Lines        4882     4937      +55     
==========================================
+ Hits         4263     4278      +15     
- Misses        619      659      +40     
Impacted Files Coverage Δ
glue_jupyter/bqplot/scatter/viewer.py 100.00% <ø> (ø)
glue_jupyter/bqplot/common/tools.py 62.07% <28.57%> (-5.34%) ⬇️
glue_jupyter/bqplot/image/viewer.py 87.50% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jfoster17
Copy link
Member Author

I think this is good-to-go. I rebased to get the CircleCI fix. @astrofrog -- would you like to do another review?

@jfoster17 jfoster17 requested a review from astrofrog July 14, 2023 01:09
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Works great in both the scatter and image viewer, thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants