Skip to content

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

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.

Sign up for GitHub

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
QuLogic merged 7 commits into from
Dec 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
723cd86
Use scatter for check boxes instead of Rectangle
chahak13 Nov 16, 2022
805f365
Specify size for the buttons to be same as radio
chahak13 Nov 16, 2022
61db294
Use correct setter to set facecolor in checkbox
chahak13 Nov 17, 2022
04d8fda
Update radio buttons to also use correct setter
chahak13 Nov 17, 2022
0f09fa1
Set rectangles and lines property for backcompat
chahak13 Nov 17, 2022
143cb9a
Create rectangles/lines when the other is accessed
chahak13 Nov 30, 2022
788a0d1
Add tests to check .rectangles and .lines fallback
chahak13 Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Set rectangles and lines property for backcompat
  • Loading branch information
@chahak13
chahak13 committed Dec 17, 2022
commit 0f09fa1318d75d5ca1cfc75b9186956b1a9e2bcc
4 changes: 2 additions & 2 deletions doc/api/prev_api_changes/api_changes_2.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,8 @@ The ``shading`` kwarg to `~matplotlib.axes.Axes.pcolor` has been
removed. Set ``edgecolors`` appropriately instead.


Functions removed from the `.lines` module
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Functions removed from the ``lines`` module
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The :mod:`matplotlib.lines` module no longer imports the
``pts_to_prestep``, ``pts_to_midstep`` and ``pts_to_poststep``
Expand Down
21 changes: 19 additions & 2 deletions lib/matplotlib/tests/test_widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,8 +1006,13 @@ def test_check_radio_buttons_image():
rb = widgets.RadioButtons(rax1, ('Radio 1', 'Radio 2', 'Radio 3'))
with pytest.warns(DeprecationWarning):
rb.circles # Trigger the old-style elliptic radiobuttons.
widgets.CheckButtons(rax2, ('Check 1', 'Check 2', 'Check 3'),
(False, True, True))
cb = widgets.CheckButtons(rax2, ('Check 1', 'Check 2', 'Check 3'),
(False, True, True))
with pytest.warns(DeprecationWarning):
# Trigger old-style Rectangle check boxes
cb.rectangles
Comment thread
chahak13 marked this conversation as resolved.
Outdated
with pytest.warns(DeprecationWarning):
cb.lines

@timhoffm timhoffm Nov 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The cb.lines line?

@timhoffm timhoffm Nov 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes.

@chahak13 chahak13 Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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!

@QuLogic QuLogic Nov 24, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@QuLogic QuLogic Nov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Nov 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@QuLogic QuLogic Dec 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@chahak13 chahak13 Dec 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Dec 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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



@check_figures_equal(extensions=["png"])
Expand All @@ -1020,6 +1025,18 @@ def test_radio_buttons(fig_test, fig_ref):
ax.text(.25, 1/3, "coffee", transform=ax.transAxes, va="center")


@check_figures_equal(extensions=["png"])
def test_check_buttons(fig_test, fig_ref):
widgets.CheckButtons(fig_test.subplots(), ["tea", "coffee"], [True, True])
ax = fig_ref.add_subplot(xticks=[], yticks=[])
ax.scatter([.15, .15], [2/3, 1/3], marker='s', transform=ax.transAxes,
s=(plt.rcParams["font.size"] / 2) ** 2, c=["none", "none"])
ax.scatter([.15, .15], [2/3, 1/3], marker='x', transform=ax.transAxes,
s=(plt.rcParams["font.size"] / 2) ** 2, c=["k", "k"])
ax.text(.25, 2/3, "tea", transform=ax.transAxes, va="center")
ax.text(.25, 1/3, "coffee", transform=ax.transAxes, va="center")


def test_slider_slidermin_slidermax_invalid():
fig, ax = plt.subplots()
# test min/max with floats
Expand Down
50 changes: 39 additions & 11 deletions lib/matplotlib/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,7 +1011,8 @@ def __init__(self, ax, labels, actives=None):
for y, label in zip(ys, labels)]

self._squares = ax.scatter(
[0.15] * len(ys), ys, marker='s', c="none", s=text_size**2,
[0.15] * len(ys), ys, marker='s', s=text_size**2,
c=["none" for _ in range(len(ys))],
Comment thread
chahak13 marked this conversation as resolved.
Outdated
linewidth=1, transform=ax.transAxes, edgecolor="k"
)
mask = [not x for x in actives]
Comment thread
chahak13 marked this conversation as resolved.
Outdated
Expand All @@ -1038,8 +1039,9 @@ def _clicked(self, event):
for i, (p, t) in enumerate(zip(self._rectangles, self.labels)):
if (t.get_window_extent().contains(event.x, event.y)
or (
p.get_x() < event.x < p.get_x() + p.get_width()
and p.get_y() < event.y < p.get_y()
p.get_x() <= pclicked[0] <= p.get_x()
+ p.get_width()
and p.get_y() <= pclicked[1] <= p.get_y()
+ p.get_height()
)):
distances[i] = np.linalg.norm(pclicked - p.get_center())
Expand Down Expand Up @@ -1081,11 +1083,10 @@ def set_active(self, index):
)
self._crosses.set_facecolor(cross_facecolors)

if hasattr(self, "_rectangles"):
for i, p in enumerate(self._rectangles):
p.set_facecolor("k" if colors.same_color(
p.get_facecolor(), colors.to_rgba("none"))
else "none")
if hasattr(self, "_lines"):
l1, l2 = self._lines[index]
l1.set_visible(not l1.get_visible())
l2.set_visible(not l2.get_visible())

if self.drawon:
self.ax.figure.canvas.draw()
Comment on lines 1086 to 1087

@chahak13 chahak13 Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@anntzer anntzer Nov 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Nov 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@chahak13 chahak13 Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@anntzer anntzer Nov 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@chahak13 chahak13 Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Expand Down Expand Up @@ -1113,24 +1114,51 @@ def disconnect(self, cid):
"""Remove the observer with connection id *cid*."""
self._observers.disconnect(cid)

@_api.deprecated("3.7")
@property
def rectangles(self):
Comment thread
tacaswell marked this conversation as resolved.
if not hasattr(self, "rectangles"):
if not hasattr(self, "_rectangles"):
ys = np.linspace(1, 0, len(self.labels)+2)[1:-1]
dy = 1. / (len(self.labels) + 1)
w, h = dy / 2, dy / 2
rectangles = self._rectangles = [
Rectangle(xy=self._squares.get_offsets()[i], width=w, height=h,
Rectangle(xy=(0.05, ys[i] - h / 2), width=w, height=h,
edgecolor="black",
facecolor=self._squares.get_facecolor()[i],
transform=self.ax.transAxes
)
for i in range(len(self.labels))
for i, y in enumerate(ys)
]
self._squares.set_visible(False)
for rectangle in rectangles:
self.ax.add_patch(rectangle)
return self._rectangles

@_api.deprecated("3.7")
@property
def lines(self):
if not hasattr(self, "_lines"):
ys = np.linspace(1, 0, len(self.labels)+2)[1:-1]
self._crosses.set_visible(False)
dy = 1. / (len(self.labels) + 1)
w, h = dy / 2, dy / 2
self._lines = []
current_status = self.get_status()
lineparams = {'color': 'k', 'linewidth': 1.25,
'transform': self.ax.transAxes,
'solid_capstyle': 'butt'}
for i, y in enumerate(ys):
x, y = 0.05, y - h / 2
l1 = Line2D([x, x + w], [y + h, y], **lineparams)
l2 = Line2D([x, x + w], [y, y + h], **lineparams)

l1.set_visible(current_status[i])
l2.set_visible(current_status[i])
self._lines.append((l1, l2))
self.ax.add_patch(l1)
self.ax.add_patch(l2)
return self._lines


class TextBox(AxesWidget):
"""
Expand Down