Skip to content

Test ScatterWidget 2D #121

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 1 commit into from
Jun 1, 2023
Merged

Conversation

ruaridhg
Copy link
Collaborator

@ruaridhg ruaridhg commented May 18, 2023

Towards #94

  • 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: Added init 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 ruaridhg requested review from dstansby and removed request for dstansby May 18, 2023 11:20
@ruaridhg ruaridhg marked this pull request as draft May 18, 2023 11:33
@ruaridhg ruaridhg changed the title Test scatter 2D example Test ScatterWidget 2D May 18, 2023
@ruaridhg ruaridhg marked this pull request as ready for review May 18, 2023 13:19
@ruaridhg ruaridhg force-pushed the ruaridhg/test_scatter branch from a28bdca to 42f9c65 Compare May 19, 2023 13:45
@ruaridhg ruaridhg force-pushed the ruaridhg/test_scatter branch 2 times, most recently from 227a33a to 1f50130 Compare May 30, 2023 10:09
@dstansby
Copy link
Member

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.

@ruaridhg
Copy link
Collaborator Author

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 ruaridhg force-pushed the ruaridhg/test_scatter branch from 29e6e9b to 8068559 Compare May 31, 2023 14:30
@ruaridhg
Copy link
Collaborator Author

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

@ruaridhg ruaridhg requested a review from dstansby May 31, 2023 14:54
Copy link
Member

@dstansby 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.

@ruaridhg ruaridhg requested a review from dstansby June 1, 2023 10:59
@ruaridhg ruaridhg force-pushed the ruaridhg/test_scatter branch 2 times, most recently from 3cebb0c to d63a0a8 Compare June 1, 2023 13:19
@ruaridhg ruaridhg enabled auto-merge June 1, 2023 13:49
@ruaridhg ruaridhg requested a review from dstansby June 1, 2023 15:20
@ruaridhg
Copy link
Collaborator Author

ruaridhg commented Jun 1, 2023

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

Copy link
Member

@dstansby dstansby left a comment

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)?

@ruaridhg
Copy link
Collaborator Author

ruaridhg commented Jun 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

@dstansby dstansby disabled auto-merge June 1, 2023 15:31
@dstansby
Copy link
Member

dstansby commented Jun 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?

@ruaridhg ruaridhg force-pushed the ruaridhg/test_scatter branch from a0758ec to 162d9dc Compare June 1, 2023 15:47
@ruaridhg ruaridhg enabled auto-merge June 1, 2023 16:01
@ruaridhg ruaridhg requested a review from dstansby June 1, 2023 16:14
@ruaridhg
Copy link
Collaborator Author

ruaridhg commented Jun 1, 2023

1 last approval hopefully!

@ruaridhg ruaridhg added this pull request to the merge queue Jun 1, 2023
@dstansby
Copy link
Member

dstansby commented Jun 1, 2023

:shipit:

Merged via the queue into matplotlib:main with commit 76dd23b Jun 1, 2023
@ruaridhg ruaridhg deleted the ruaridhg/test_scatter branch June 2, 2023 08:28
@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.

Fix features_scatter_widget unpredictable behaviour
2 participants