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

pyqt scatter widget #270

Open
wants to merge 85 commits into
base: main
Choose a base branch
from
Open

pyqt scatter widget #270

wants to merge 85 commits into from

Conversation

fjorka
Copy link

@fjorka fjorka commented Jul 11, 2024

Description

This PR introduces a complete Scatter Widget based on PyQtGraph, intended to replace the Matplotlib-based Scatter Widget.

Rationale

  • Expected improved performance for large datasets.
  • Smooth navigation for zooming and panning within the plot.
  • Increased flexibility in customization.

Functionality

  • It preserves all functionality of the previous widget.
  • Documentation notebooks updated with the information and screenshots of the new widget.
  • New functionality includes:
    • Separate color controls for continuous and discrete data:
      • Continuous color control offers:
        • Histogram of values chosen for color.
        • Ability to change LUT.
        • Control over contrast.
      • Discrete color control offers:
        • Scrollable legend of colors.
        • Ability to change the color for a selected class.
    • Pseudohistogram plot when only one axis is defined.
    • Highlight of nearby data points under cursor.
    • Dynamic status display of cursor position and highlighted data points.
    • Expanded ROI annotations:
      • Lasso or rectangle tool (useful for thresholding).
      • Ability to use several disjointed ROIs for annotation.
      • Modifiable ROIs.
    • Dialog to choose Annotation name (previously Export).
    • Saving options:
      • When started with AnnData, the Save button opens a path dialog to choose a location for an AnnData file (supports h5ad, zarr, and csv).
      • When opened with the Napari viewer, it mimics the behavior of the Annotation Widget:
        • With backed sdata, Save will modify sdata of the selected layer.
        • Without backed sdata, no saving action.
      • The widget displays the status and saving possibility when started.

Next Steps Beyond This PR

  • Flip the Y-axis (scatterplot in spatial coordinates is flipped relative to the image).
  • Signal point highlight to the main viewer for quick connection between visualizations in different dimensions.
  • Highlight object in the main Napari window to trigger highlight in the scatterplot.
  • Integrate Annotation Widget functionality into the Scatter Widget (the Annotation Widget facilitates annotating multiple classes within a single column).
  • Performance improvements (1. How points belonging to polygons is checked.)

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@fjorka fjorka marked this pull request as draft July 11, 2024 13:51
widget = PlotWidget(None, model)
qtbot.addWidget(widget)
yield widget
widget.deleteLater()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once you have added widget to qtbot, then you should not use deleteLater.

Copy link
Author

@fjorka fjorka Aug 26, 2024

Choose a reason for hiding this comment

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

Hi @Czaki !

I do see the error:

E       AssertionError: Some instance of QtViewer is not properly cleaned in one of previous test. For easier debug one may use --save-leaked-object-graph flag for pytest to get graph of leaked objects. If you use qtbot (from pytest-qt) to clean Qt objects after test you may need to switch to manual clean using `deleteLater()` and `qtbot.wait(50)` later.
E       assert 1 == 0

when I run pytest locally (Python 3.10, Windows).

You are correct, the additional deleteLater is unnecessary. I added it following the error message as I was getting desperate. I will remove it to avoid confusion.

Now, the minimum example to reproduce the error is:

import pytest
from napari_spatialdata._model import DataModel
from napari_spatialdata._scatterwidgets import PlotWidget

@pytest.fixture
def plot_widget(qtbot):
    """Fixture for creating a PlotWidget instance."""
    model = DataModel()
    widget = PlotWidget(None, model)
    qtbot.addWidget(widget)
    yield widget

def test_initialization(plot_widget):
    """Test initialization of PlotWidget."""
    assert plot_widget is not None

def test_elementwidget(make_napari_viewer):
    _ = make_napari_viewer()
    assert 1 == 1

However, it's not the problem specific to this branch. If I remove all tests from test_scatterwidgets.py, the error persists:
image

Finally, the error is triggered only once. If I run the fake test (the code pasted in above) as test_aaa.py before any other test, the error appears in the fake test but is not triggered again by any of the original tests:
image

Any ideas what is going on here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not Idea. It is knowledge created in pain.
You may also see this error in tests executed on main.
Will try to fix this.
In general, the problem comes from previous tests that are spawning the Viewer.

in a general make_napari_viewer fixture has few weak spots (but writing a test without using it, may lead to even more spectacular failures)

I even have opened some PR to fix it, but it is really time-consuming.
for example napari/napari#6745

@fjorka
Copy link
Author

fjorka commented Aug 26, 2024

  • is it possible to remove the white border around the scatterplot points?

Replaced with black (invisible) border when color is defined - 1acdd85

  • scatterwidget_annotation.ipynb is missing from docs/index.md

Included in 9191c7e

  • Minor thing, can you please update the changelog? Thanks.

Suggestion in 973ebcc. Needs to be finalized.

@fjorka
Copy link
Author

fjorka commented Aug 28, 2024

The notebooks start with the instruction:

There are two options to install napari-spatialdata:

(1) Run `pip install napari-spatialdata`
or,
(2) Clone this [repo](https://github.com/scverse/napari-spatialdata) and run `pip install -e .`

However, following this instruction and starting napari (or scatter widget) will give an error ImportError: No Qt bindings could be found. What about asking explicitly for napari installation first d40fb7c which will inform a user about Qt?

@fjorka
Copy link
Author

fjorka commented Aug 28, 2024

To do:

  • new screenshots for the notebooks (after approving the new look by @LucaMarconato )
  • add explanations in scatterwidget.ipynb and scatterwidget_annotation.ipynb about starting with AnnData and options for saving
  • (possibly) add a warning dialog about overwriting sdata object

@LucaMarconato
Copy link
Member

TODO for me: dataset

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.

4 participants