test_scatter.py: Added astronaut image and an inverted *-1 astronaut image to the viewer and made sure both image layers were selected before calling the ScatterWidget
test_scatter.png added
scatter.py: Addedinit to ScatterWidget class to allow self.updates_layers(None) to add the selected layers (shouldn't have an effect on other parts of the code?)

For ScatterWidget 2D only (still to test 3D data and FeaturesScatterWidget)
Fixes#129

ruaridhg requested review fromdstansby and removed request fordstansby

May 18, 2023 11:20

ruaridhg marked this pull request as draft

May 18, 2023 11:33

ruaridhg changed the title~~Test scatter 2D example~~Test ScatterWidget 2D

May 18, 2023

ruaridhg marked this pull request as ready for review

May 18, 2023 13:19

ruaridhg mentioned this pull request

May 18, 2023

Add test_scatter.png so tox passes in other branch#123

Closed

ruaridhg force-pushed theruaridhg/test_scatter branch froma28bdca to42f9c65Compare

May 19, 2023 13:45

ruaridhg mentioned this pull request

May 19, 2023

Clean up layer selection API#128

Merged

ruaridhg force-pushed theruaridhg/test_scatter branch 2 times, most recently from227a33a to1f50130Compare

May 30, 2023 10:09

Copy link

Member

dstansby commentedMay 31, 2023

It looks like you're tackling two different tests in this PR (scatter and features scatter)? Iis one of them working and one of them not? If one of them is working, I'd suggest putting that in a separate PR that we can merge straight away. Then we can keep moving, and it will make this PR easier to debug as it will only be concerned with one test.

Copy link

CollaboratorAuthor

ruaridhg commentedMay 31, 2023

It looks like you're tackling two different tests in this PR (scatter and features scatter)? Iis one of them working and one of them not? If one of them is working, I'd suggest putting that in a separate PR that we can merge straight away. Then we can keep moving, and it will make this PR easier to debug as it will only be concerned with one test.

Neither of them currently work, although features scatter at least can plot something (even if what it is plotting is inconsisent). I agree that they should be separate PRs so I will try to untangle them

ruaridhg force-pushed theruaridhg/test_scatter branch from29e6e9b to8068559Compare

May 31, 2023 14:30

Copy link

CollaboratorAuthor

ruaridhg commentedMay 31, 2023

#137 solved inconsistent ordering problem. Partially addresses#129 with other behaviour perhaps desired

ruaridhg requested a review fromdstansby

May 31, 2023 14:54

dstansby requested changes

May 31, 2023

View reviewed changes

Copy link

Member

dstansby left a comment

Choose a reason for hiding this comment

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

Looks 👍 overall, but we want to keep the number of layers features scatter takes as input (1), and plot two different features stored on that one layer against each other.

src/napari_matplotlib/scatter.py OutdatedShow resolvedHide resolved

ruaridhg requested a review fromdstansby

June 1, 2023 10:59

ruaridhg force-pushed theruaridhg/test_scatter branch 2 times, most recently from3cebb0c tod63a0a8Compare

June 1, 2023 13:19

ruaridhgenabled auto-merge

June 1, 2023 13:49

dstansby requested changes

Jun 1, 2023

View reviewed changes

src/napari_matplotlib/tests/test_scatter.py OutdatedShow resolvedHide resolved

tox.ini OutdatedShow resolvedHide resolved

ruaridhg requested a review fromdstansby

June 1, 2023 15:20

Copy link

CollaboratorAuthor

ruaridhg commentedJun 1, 2023

Made the suggested changes including reverting back the tox.ini so just requested a re-review as merging is blocked

dstansby requested changes

Jun 1, 2023

View reviewed changes

Copy link

Member

dstansby left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I think there's still code that can be removed here (see my suggestions)?

src/napari_matplotlib/tests/test_scatter.py OutdatedShow resolvedHide resolved

Copy link

CollaboratorAuthor

ruaridhg commentedJun 1, 2023

I think there's still code that can be removed here (see my suggestions)?

Ah yes, got rid of the image layer now and tidying up of remaining comments

dstansbydisabled auto-merge

June 1, 2023 15:31

dstansby approved these changes

Jun 1, 2023

View reviewed changes

Copy link

Member

dstansby commentedJun 1, 2023

One final request (sorry!), could you squash this all into one commit so we don't add any more test figures changes than we need to in the commit history when it's merged into main?