Use scatter for check boxes and set facecolors correctly in check boxes and radio buttons #24474

chahak13 · 2022-11-16T04:31:12Z

With the current implementation, the boxes get stretched into rectangles if the aspect ratio is not maintained. To overcome this, the boxes are now created using scatter instead to maintain their shapes.

Documentation and Tests

Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).
New plotting related features are documented with examples.

Release Notes

New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

With the current implementation, the boxes get stretched into rectangles if the aspect ratio is not maintained. To overcome this, the boxes are now created using scatter instead to maintain their shapes.

chahak13 · 2022-11-16T04:37:45Z

lib/matplotlib/widgets.py

        if self.drawon:
            self.ax.figure.canvas.draw()


@anntzer I have a preliminary implementation for square check boxes but I'm running into one small issue. When I try running this, everything works as intended when we change the figure size (the check boxes remain square), but the unit test fails. For some reason, calling set_active(0) in code doesn't change the status of the checkboxes. The issue is arising in these lines. The status changes correctly based on the code that I added but after executing self.ax.figure.canvas.draw(), the colors for all the scatter markers change back to what they were initially set to be, and not the changed ones. This is happening only in the unit tests and not while actually checking it using a GUI backend. Any ideas/suggestions?

I feel like I'm missing something but can't see it right now.

If you do some print-debugging and grep for places that could touch facecolors, you'll see that Collection.update_scalarmappable gets called at some point which causes the facecolors to be reset, but you'll need to figure out by yourself why (because I don't actually know immediately).

That helps, thanks! I'll try figuring out why this is happening. The main reason why I was getting confused was that it happened only when set_active was explicitly called but worked as intended with the actual figure. Should be able to figure this out now though, hopefully.

@anntzer this is actually true for the radio buttons too! Small code to reproduce (based on the checkbox example)

radio = RadioButtons(rax, labels, active=1, activecolor="k") print(radio.value_selected) print(radio._buttons.get_facecolor()) print("Changing active button from 1 to 0") radio.set_active(0) print(radio.value_selected) print(radio._buttons.get_facecolor())

Output:

4 Hz [[0. 0. 0. 0.] [0. 0. 0. 1.] [0. 0. 0. 0.]] Changing active button from 1 to 0 2 Hz [[0. 0. 0. 0.] [0. 0. 0. 1.] [0. 0. 0. 0.]]

This shows that the facecolors for the buttons don't change even after set_active(0) is called, even though value_selected is correct. The problem arises because of these lines

matplotlib/lib/matplotlib/collections.py

Lines 842 to 845 in a9c9620

# The flags are initialized to None to ensure this returns True

# the first time it is called.

edge0 = self._edge_is_mapped

face0 = self._face_is_mapped

Since it sets the flags such that it returns True the first time,

matplotlib/lib/matplotlib/collections.py

Lines 894 to 900 in a9c9620

else:

self._set_facecolor(self._original_facecolor)

if self._edge_is_mapped:

self._edgecolors = self._mapped_colors

else:

self._set_edgecolor(self._original_edgecolor)

self.stale = True

set the original colors again. Since update_scalarmappable gets called the first time they're drawn, when creating a figure, all functionalities work as everything works as expected after the second call. Even in this case, if there are any set_active calls after the first one, they work fine.

Thanks for the investigation, I have opened #24479 to track this. Sorry for sending you down this rabbit hole, perhaps this was not so easy after all...

Thanks, it was fun. But now with the question on hand, this bug stops me to run get_status for the check boxes correctly. I guess one workaround could be that I maintain an active array similar to value_active in radio buttons but that'll be a hack and I'm on the fence about having an extra array to handle that when it can be done with the colors itself. That'll pass the tests though, and everything works in a figure anyway. Suggestions?

lib/matplotlib/widgets.py

tacaswell · 2022-11-16T23:09:32Z

I do not think that going with scatter is the correct approach here. Looking at the internals of Rectangle we have a (private) knob to account for the aspect ratio

matplotlib/lib/matplotlib/patches.py

Lines 720 to 726 in a9c9620

    
           # Required for RectangleSelector with axes aspect ratio != 1 
        
           # The patch is defined in data coordinates and when changing the 
        
           # selector with square modifier and not in data coordinates, we need 
        
           # to correct for the aspect ratio difference between the data and 
        
           # display coordinate systems. Its value is typically provide by 
        
           # Axes._get_aspect_ratio() 
        
           self._aspect_ratio_correction = 1.0

I suspect that making sure this gets set on draw is a much simpler way to solve this problem.

chahak13 · 2022-11-17T03:06:24Z

@tacaswell Just making sure that I'm understanding correctly, but if we do that then we're kinda working on the current implementation itself, right? In that case, we'll also have to make a call on should the checkbox size increase, while maintaining the square shape or not?

timhoffm · 2022-11-17T05:24:15Z

I do not think that going with scatter is the correct approach here.

@tacaswell why? Using markers instead of cobbling the box together from basic artists has some appeal. You don't have to bother with aspect. You get pixel snapping. You have only a single artist to change for updating the check state. And you can switch to more complex markers like a check symbol relatively easy.

Note also that @anntzer did the same thing in #24455 with radio buttons.

ksunden · 2022-11-17T07:34:55Z

lib/matplotlib/widgets.py

+        if colors.same_color(
+                self._crosses.get_facecolor()[index], colors.to_rgba("none")
+        ):
+            self._crosses.get_facecolor()[index] = colors.to_rgba("k")
+        else:
+            self._crosses.get_facecolor()[index] = colors.to_rgba("none")


The point made in #24479 is that editing the output of the getter function is not guaranteed to keep those changes (it works, sometimes, because it is not a copy that is returned, but there are other mechanisms at play)

more properly this would be:

facecolors = self._crosses.get_faceolor() facecolors[index] = colors.to_rgba("k") # or "none" self._crosses.set_facecolor(facecolors)

This calls the setter which updates the internal state properly

(This also does need to be corrected in the radio button implementation)

Thanks! Just went through the discussion too. Don't know how I missed the setter. Will update this!

PS: @anntzer do you mind if I also update the radio button implementation? Or should that be a different PR?

Sure, please do it here too.

anntzer · 2022-11-17T09:56:19Z

From a quick look, _aspect_ratio_correction, introduced in #20839 for the rectangle selector widget, seems actually brittle, as 1) it will basically never work for polar plots (not a concern here, but one for rectangle selectors in screen space), and 2) it makes one of the two axises the "main" one relative to the other (I think for circles the radius will effectively be the radius in relative units along the x axis, not along the y axis), whereas the proper thing to do would be to have a radius in pixels (basically proportional to the font size, to make the radio button close to the size of the label).
If we really want to do this with individual artists we'd need Circle to support having different transforms for center and radius (a bit like how PathCollections have both a transform and an offset_transform, which is what we're effectively relying on here).

tacaswell · 2022-11-17T15:24:48Z

Fair enough. I missed that we did this for radio buttons and I am biased towards the minimal changes to solve the problem. The consensus is clearly in favor of this so I take back my comment.

anntzer

I would rather not change the test image, see how I ensured that it still uses the old radiobuttons. We can always reconsider and change both at the same time once the deprecation elapses, but the better way to test the new version is probably to use check_figures_equal like I did in test_radio_buttons.

lib/matplotlib/widgets.py

chahak13 · 2022-11-17T20:42:20Z

I would rather not change the test image, see how I ensured that it still uses the old radiobuttons. We can always reconsider and change both at the same time once the deprecation elapses, but the better way to test the new version is probably to use check_figures_equal like I did in test_radio_buttons.

Ah gotcha. I'll take a look at your PR and do that. Thanks.

chahak13 · 2022-11-17T22:17:57Z

The failing check is unhappy because I changed an image and reverted the test image for test_check_radio_buttons. Is there a recommended way to resolve that? Also, I'm not sure why the docs are failing, I did not make any changes there.

ksunden · 2022-11-17T22:33:05Z

Docs test is because we fail on warnings and you got this warning:

https://app.circleci.com/pipelines/github/matplotlib/matplotlib/20207/workflows/95614b42-e1b0-4ead-850e-66805085a944/jobs/72159?invite=true#step-113-1153

which is a poor interaction with an old changelog entry which didn't use the full name of a reference.

https://github.com/matplotlib/matplotlib/blame/v3.6.2/doc/api/prev_api_changes/api_changes_2.1.0.rst#L425

This line should probably be updated to simply be lines (no reference, but codestyled) as it is in a heading (and the proper reference link is right below it anyway...)

anywho, because you are defining a function called "lines" and that reference was not fully specified, it had worked because there was only one target for "lines" previously, but now there is more than one.

ksunden · 2022-11-17T22:39:03Z

As for the PR cleanliness check, are you familiar with git rebase -i?

here is an article that goes over the basics, probably better than I could in an off hand github comment (though admittedly I only skimmed this article to make sure it wasn't outright incorrect):

https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history

Feel free to ask any additional clarifying questions, its a relatively complicated (and powerful) edge case of git

Essentially rather than committing, changing and then reverting that image in separate commits, squashing out the changes to the image entirely will make it so that there is not an extra version of the image in the canonical git history.

chahak13 · 2022-11-17T22:39:44Z

Ah okay. That makes sense. Though, should I do anything in this PR to resolve that? That's not close to anything I'm working on here.

chahak13 · 2022-11-17T22:41:18Z

Yes, I'm familiar with rebase, just did not want to go rebase stuff without knowing if that was fine since some people prefer not to do that. I'll rebase the commits to remove the image issue. Thanks!

ksunden · 2022-11-17T22:41:35Z

since its a one line change that while it is unrelated to what you were directly working on, it is elevated to a warning by what you were doing, I'd just go ahead and fix it here to keep CI happy rather than opening a separate PR

PR matplotlib#24474 introduces a (to be deprecated) property called `lines`. Because of that, the documentation in a previous api change doc which just used `.lines` instead of :mod:`matplotlib.lines` couldn't be built properly as it was unable to resolve the names. This changes the old doc to the correct module link.

doc/api/prev_api_changes/api_changes_2.1.0.rst

timhoffm · 2022-11-21T19:13:48Z

lib/matplotlib/tests/test_widgets.py

+    with pytest.warns(DeprecationWarning):
+        # Trigger old-style Rectangle check boxes
+        cb.rectangles
+        cb.lines


This line is never reached. You need to put in in a separate warns() context.

The cb.lines line?

Ah okay, thanks. I realized a funny thing then. Accessing rectangles is all that's required since that's all that changes from the older reference image with the rectangles. The line crosses and the scatter crosses are the exact same. Anyway, I've updated it, thanks!

I think you'd need to create a new CheckButtons object to check that the cb.lines is falling back correctly.

We should still check both, I think. Also, does accessing the other object trigger a second warning unnecessarily?

No yeah, I agree about checking both, just that they should be different independent tests and not on this one. About the second warning, yeah, that's a good point. Do you know of a straight-forward way to call the lines property from inside rectangles without triggering the API deprecation decorator warning?

You can wrap it in a _api.suppress_matplotlib_deprecation_warning context, or put the code in a separate private function that's called from both.

I'll take a look at that. Thanks, did not know about the context!

@QuLogic I've updated and added 2 tests for checking the fallbacks. Let me know if this makes sense now!

doc/api/next_api_changes/deprecations/24474_CM.rst

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

chahak13 · 2022-12-06T18:12:31Z

@QuLogic let me know when you get a chance to look at this. Thanks!

tacaswell · 2022-12-15T16:33:15Z

Running this example: https://github.com/matplotlib/matplotlib/blob/main/examples/widgets/check_buttons.py

The CheckButtons work and show the initial state correctly, but the x 's do not come back when you go from off -> on.

tacaswell · 2022-12-15T16:40:51Z

I'm going to push this to 3.8, but if it is indeed ready by the end of the year I am 👍🏻 on it getting merged and being re-milestoned to 3.7.

chahak13 · 2022-12-15T18:14:41Z

That issue was not supposed to be there anymore....taking a look right away. Sorry this is taking too long, I keep missing small things with this one for some reason.

chahak13 · 2022-12-15T18:27:08Z

@tacaswell corrected that. Everything seems to be working locally. Can you check once? Thanks!

No changes to test images are left.

tacaswell · 2022-12-16T18:51:07Z

@chahak13 Could you squash this to a smaller number of commits?

The rectangles/lines attributes exist for backcompat and the expectation would be that when either one of them is accessed, the other would also be set in the way the older method does. So, whenever either of the property is accessed for the first time, it also sets the other property. This way, it maintains consistency with the old API.

chahak13 · 2022-12-17T04:30:37Z

That should do it. I've kept the major, important commits and squashed all changes into them.

lib/matplotlib/widgets.py

QuLogic

I think I'm okay with this now, but you may want to squash it down again?

doc/api/next_api_changes/deprecations/24474_CM.rst

chahak13 · 2022-12-22T04:59:26Z

@QuLogic done. Should be good.

Use scatter for check boxes instead of Rectangle

723cd86

With the current implementation, the boxes get stretched into rectangles if the aspect ratio is not maintained. To overcome this, the boxes are now created using scatter instead to maintain their shapes.

chahak13 commented Nov 16, 2022

View reviewed changes

oscargus reviewed Nov 16, 2022

View reviewed changes

lib/matplotlib/widgets.py Show resolved Hide resolved

anntzer mentioned this pull request Nov 16, 2022

[Bug]: Updating the facecolors of a collection before its first draw has no effect #24479

Open

Specify size for the buttons to be same as radio

805f365

ksunden reviewed Nov 17, 2022

View reviewed changes

chahak13 added 2 commits November 17, 2022 12:36

Use correct setter to set facecolor in checkbox

61db294

Update radio buttons to also use correct setter

04d8fda

chahak13 changed the title ~~Use scatter for check boxes instead of Rectangle~~ Use scatter for check boxes and set facecolors correctly in check boxes and radio buttons Nov 17, 2022

anntzer previously requested changes Nov 17, 2022

View reviewed changes

anntzer reviewed Nov 17, 2022

View reviewed changes

lib/matplotlib/widgets.py Show resolved Hide resolved

chahak13 force-pushed the make_checkbox_square_again branch from 31e9a1f to d2b175f Compare November 17, 2022 22:50

ksunden reviewed Nov 17, 2022

View reviewed changes

doc/api/prev_api_changes/api_changes_2.1.0.rst Outdated Show resolved Hide resolved

timhoffm reviewed Nov 21, 2022

View reviewed changes

rcomer mentioned this pull request Nov 25, 2022

[Bug]: CheckBoxes should be square, not rectangular #24471

Closed

rcomer linked an issue Nov 25, 2022 that may be closed by this pull request

[Bug]: CheckBoxes should be square, not rectangular #24471

Closed

anntzer mentioned this pull request Nov 30, 2022

Add blitting support to button widgets #23457

Merged

6 tasks

tacaswell added this to the v3.7.0 milestone Nov 30, 2022

QuLogic reviewed Dec 2, 2022

View reviewed changes

tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 15, 2022

tacaswell modified the milestones: v3.8.0, v3.7.0 Dec 15, 2022

tacaswell approved these changes Dec 16, 2022

View reviewed changes

chahak13 added 2 commits December 17, 2022 09:35

Set rectangles and lines property for backcompat

0f09fa1

chahak13 force-pushed the make_checkbox_square_again branch from 42b4a89 to ba30bdf Compare December 17, 2022 04:21

QuLogic reviewed Dec 17, 2022

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

QuLogic approved these changes Dec 21, 2022

View reviewed changes

doc/api/next_api_changes/deprecations/24474_CM.rst Outdated Show resolved Hide resolved

Add tests to check .rectangles and .lines fallback

788a0d1

chahak13 force-pushed the make_checkbox_square_again branch from dbd882b to 788a0d1 Compare December 22, 2022 04:58

QuLogic merged commit cd8420a into matplotlib:main Dec 22, 2022

QuLogic mentioned this pull request Mar 19, 2024

Horizontal radio buttons #13374

Open

6 tasks

	# The flags are initialized to None to ensure this returns True
	# the first time it is called.
	edge0 = self._edge_is_mapped
	face0 = self._face_is_mapped

	else:
	self._set_facecolor(self._original_facecolor)
	if self._edge_is_mapped:
	self._edgecolors = self._mapped_colors
	else:
	self._set_edgecolor(self._original_edgecolor)
	self.stale = True

Use scatter for check boxes and set facecolors correctly in check boxes and radio buttons #24474

Use scatter for check boxes and set facecolors correctly in check boxes and radio buttons #24474

Conversation

chahak13 commented Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahak13 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 16, 2022

chahak13 commented Nov 17, 2022

timhoffm commented Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

anntzer commented Nov 17, 2022

tacaswell commented Nov 17, 2022

anntzer left a comment

Choose a reason for hiding this comment

chahak13 commented Nov 17, 2022

chahak13 commented Nov 17, 2022

ksunden commented Nov 17, 2022

ksunden commented Nov 17, 2022 • edited Loading

chahak13 commented Nov 17, 2022

chahak13 commented Nov 17, 2022

ksunden commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahak13 commented Dec 6, 2022

tacaswell commented Dec 15, 2022

tacaswell commented Dec 15, 2022

chahak13 commented Dec 15, 2022

chahak13 commented Dec 15, 2022

tacaswell commented Dec 16, 2022

chahak13 commented Dec 17, 2022

QuLogic left a comment

Choose a reason for hiding this comment

chahak13 commented Dec 22, 2022

chahak13 commented Nov 16, 2022 •

edited

Loading

chahak13 Nov 16, 2022 •

edited

Loading

timhoffm commented Nov 17, 2022 •

edited

Loading

anntzer Nov 17, 2022 •

edited

Loading

ksunden commented Nov 17, 2022 •

edited

Loading