Skip to content

Add scatter figure test #137

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 2 commits into from
May 31, 2023
Merged

Add scatter figure test #137

merged 2 commits into from
May 31, 2023

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented May 31, 2023

Adds a figure test for scattering two images against each other.

The fix to previous issues we were having was adding the line widget = ScatterWidget(viewer). For some reason, storing the widget in a variable instead of just doing figure = ScatterWidget(viewer).figure makes the callbacks work properley... I'm not entirely sure why, and I don't propose to enter that rabbit hole.

@dstansby dstansby requested a review from ruaridhg May 31, 2023 12:43
Copy link
Collaborator

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

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

Works for me locally!

@ruaridhg
Copy link
Collaborator

I think the tests are failing because a similar issue to #129 where the order is inconsistent so generating the test figure can switch from astronaut vs astronaut_reversed to astronaut_reversed vs astronaut meaning the image comparison fails

@dstansby
Copy link
Member Author

I'm guessing that's because the selected layers are stored in a set(), which doesn't guarentee the order of layers. Let me see if I can think of a fix for the tests.

@dstansby dstansby marked this pull request as ready for review May 31, 2023 13:16
@dstansby
Copy link
Member Author

To give the layers a deterministic order, I've sorted them by their name - do you think that's a good soluiton?

@dstansby dstansby requested a review from ruaridhg May 31, 2023 13:32
Copy link
Collaborator

@ruaridhg ruaridhg left a comment

Choose a reason for hiding this comment

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

It works for the tests, but does that mean that it will always plot based on alphabetical order rather than order of selecting within napari? I guess it's still probably worth merging for now

@ruaridhg ruaridhg added this pull request to the merge queue May 31, 2023
Merged via the queue into matplotlib:main with commit 1af4e64 May 31, 2023
@ruaridhg ruaridhg mentioned this pull request May 31, 2023
@dstansby dstansby added the Tests label Jun 13, 2023
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.

2 participants