Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 7 commits intomatplotlib:mainfromchahak13:make_checkbox_square_again
Dec 22, 2022

Conversation

@chahak13
Copy link
Contributor

@chahak13chahak13 commentedNov 16, 2022
edited
Loading

ref:#24471

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 (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

With the current implementation, the boxes get stretched into rectanglesif the aspect ratio is not maintained. To overcome this, the boxes arenow created using scatter instead to maintain their shapes.
Comment on lines 1087 to 1088
ifself.drawon:
self.ax.figure.canvas.draw()
Copy link
ContributorAuthor

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, callingset_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 executingself.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?

Copy link
ContributorAuthor

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.

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

Copy link
ContributorAuthor

@chahak13chahak13Nov 16, 2022
edited
Loading

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.

Copy link
ContributorAuthor

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 02 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 afterset_active(0) is called, even thoughvalue_selected is correct. The problem arises because of these lines

# 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,

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

set the original colors again. Sinceupdate_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 anyset_active calls after the first one, they work fine.

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

Copy link
ContributorAuthor

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 runget_status for the check boxes correctly. I guess one workaround could be that I maintain an active array similar tovalue_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?

@tacaswell
Copy link
Member

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

# 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 ondraw is a much simpler way to solve this problem.

@chahak13
Copy link
ContributorAuthor

@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
Copy link
Member

timhoffm commentedNov 17, 2022
edited
Loading

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.

Comment on lines 1074 to 1079
ifcolors.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")
Copy link
Member

Choose a reason for hiding this comment

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

The point made in#24479 is that editing the output of the getter function is not guaranteed tokeep 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)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@anntzeranntzerNov 17, 2022
edited
Loading

Choose a reason for hiding this comment

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

Sure, please do it here too.

chahak13 reacted with thumbs up emoji
@anntzer
Copy link
Contributor

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
Copy link
Member

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.

@chahak13chahak13 changed the titleUse scatter for check boxes instead of RectangleUse scatter for check boxes and set facecolors correctly in check boxes and radio buttonsNov 17, 2022
anntzer
anntzer previously requested changesNov 17, 2022
Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

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.

chahak13 reacted with thumbs up emoji
@chahak13
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

The failing check is unhappy because I changed an image and reverted the test image fortest_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
Copy link
Member

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 belines (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
Copy link
Member

ksunden commentedNov 17, 2022
edited
Loading

As for the PR cleanliness check, are you familiar withgit 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 ofgit

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
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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
Copy link
Member

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

chahak13 reacted with thumbs up emoji

@chahak13chahak13force-pushed themake_checkbox_square_again branch from31e9a1f tod2b175fCompareNovember 17, 2022 22:50
chahak13 added a commit to chahak13/matplotlib that referenced this pull requestNov 17, 2022
PRmatplotlib#24474 introduces a (to be deprecated) property called `lines`.Because of that, the documentation in a previous api change doc whichjust used `.lines` instead of :mod:`matplotlib.lines` couldn't be builtproperly as it was unable to resolve the names. This changes the old docto the correct module link.
withpytest.warns(DeprecationWarning):
# Trigger old-style Rectangle check boxes
cb.rectangles
cb.lines
Copy link
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 separatewarns() context.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thecb.lines line?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
ContributorAuthor

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!

Copy link
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 newCheckButtons object to check that thecb.lines is falling back correctly.

timhoffm reacted with thumbs up emoji
Copy link
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?

Copy link
ContributorAuthor

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 thelines property from insiderectangles without triggering the API deprecation decorator warning?

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

Copy link
ContributorAuthor

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!

Copy link
ContributorAuthor

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!

@tacaswelltacaswell added this to thev3.7.0 milestoneNov 30, 2022
@chahak13
Copy link
ContributorAuthor

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

@tacaswell
Copy link
Member

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

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

@tacaswell
Copy link
Member

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.

@tacaswelltacaswell modified the milestones:v3.7.0,v3.8.0Dec 15, 2022
@chahak13
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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

@tacaswelltacaswell modified the milestones:v3.8.0,v3.7.0Dec 15, 2022
@tacaswelltacaswell dismissedanntzer’sstale reviewDecember 16, 2022 18:39

No changes to test images are left.

@tacaswell
Copy link
Member

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

The rectangles/lines attributes exist for backcompat and the expectationwould be that when either one of them is accessed, the other would alsobe set in the way the older method does. So, whenever either of theproperty is accessed for the first time, it also sets the otherproperty. This way, it maintains consistency with the old API.
@chahak13chahak13force-pushed themake_checkbox_square_again branch from42b4a89 toba30bdfCompareDecember 17, 2022 04:21
@chahak13
Copy link
ContributorAuthor

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

Copy link
Member

@QuLogicQuLogic 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 I'm okay with this now, but you may want to squash it down again?

@chahak13chahak13force-pushed themake_checkbox_square_again branch fromdbd882b to788a0d1CompareDecember 22, 2022 04:58
@chahak13
Copy link
ContributorAuthor

@QuLogic done. Should be good.

@QuLogicQuLogic merged commitcd8420a intomatplotlib:mainDec 22, 2022
@QuLogicQuLogic mentioned this pull requestMar 19, 2024
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@anntzeranntzeranntzer left review comments

@ksundenksundenksunden left review comments

@timhoffmtimhoffmtimhoffm left review comments

@oscargusoscargusoscargus left review comments

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

[Bug]: CheckBoxes should be square, not rectangular

7 participants

@chahak13@tacaswell@timhoffm@anntzer@ksunden@QuLogic@oscargus

[8]ページ先頭

©2009-2025 Movatter.jp