FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Resolves different edgecolor and hatch color in bar plot by Vashesh08 · Pull Request #26993 · matplotlib/matplotlib · GitHub

Repository navigation

Resolves different edgecolor and hatch color in bar plot - #26993

Closed
Vashesh08 wants to merge 11 commits into
matplotlib:mainfrom
Vashesh08:Issue26074
Closed

Vashesh08 wants to merge 11 commits into
matplotlib:mainfrom
Vashesh08:Issue26074

Conversation

Vashesh08 commented Oct 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR summary

Fixes #26074
Continued From #26683

PR checklist

story645 left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi, thanks for following up. I'm not sure if your tests are in draft form, but they're currently not testing anything because your reference and actual figures are using the same code to make the same image. You may need a mix of comparison and equals_tests here https://matplotlib.org/devdocs/devel/testing.html#writing-an-image-comparison-test

Comment thread lib/matplotlib/patches.py
Comment thread lib/matplotlib/patches.py
Comment thread lib/matplotlib/patches.py Outdated
Comment thread lib/matplotlib/patches.py Outdated
Comment thread lib/matplotlib/tests/test_axes.py Outdated

Copy link
Copy Markdown
Member

Hi @Vashesh08 , it looks like you pulled in a number of unrelated commits into your branch. You will probably need to rebase your branch to leave only your commits. If you run into trouble and need help, let us know.

Copy link
Copy Markdown
Contributor Author

@melissawm I think I removed the unrelated commits. Thanks for the docs!

story645 left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi,
I'm really sorry I dropped the ball on finishing this up. If you've got the time, my major question/concern/thing I should have flagged earlier (sorry :wince:) is that the hatchcolor shouldn't be set inside edgecolor. I think seperating it out so that hatchcolor is only set inside set_hatchcolor, even when set to the edge color, should clean up some of the logic chains happening now and will hopefully get rid of the need for a second set_hatch_color variable.

Comment thread lib/matplotlib/patches.py
dstansby marked this pull request as draft January 14, 2024 16:45

Copy link
Copy Markdown
Contributor Author

I'm extremely sorry for the delay in responding. I missed out on your response.

The primary concern I had was how should I tackle the hatchcolor when both hatchcolor and edgecolor values both are specified. Since these values are updating using _internal_update method, which makes the order of these parameters important. In order to tackle this, I had to introduced a class variable self.set_hatch_color and local function variables set_hatchcolor_from_edgecolor.

the hatchcolor shouldn't be set inside edgecolor
I think the way this would have to be done would be passing an additional parameter to set_hatchcolor function. Although that would make a redundant variable but I think it would clean up the logic.

I will be pushing these changes. Lmk if you still think there are room for any more improvements.

Vashesh08 marked this pull request as ready for review February 13, 2024 16:03
Vashesh08 requested a review from story645 February 14, 2024 13:56

story645 left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I might be missing something, but I don't see why this isn't just a fallback chain. Also sorry it took so long to get back to you.

Comment thread lib/matplotlib/patches.py
Comment thread lib/matplotlib/patches.py

story645 commented Feb 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

hi, we discussed this on the call this week b/c I was worried and @ksunden and @QuLogic both suggested that there's a way to inherit the rcparam from edgecolor so that you can reduce the fallback logic and that legend.facecolor already tracks legend.edgecolor and you can maybe use that as a model. What that means is it solves the black default or intentional problem so

Also thanks for your patience and perseverance on this.

Copy link
Copy Markdown
Contributor

@Vashesh08 are you coming back to this? Or should I give it a go

Vashesh08 commented Apr 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Impaler343
I have been a little busy and haven't had enough time to work on this. Please go ahead.

Impaler343 mentioned this pull request Apr 19, 2024
6 tasks done

Copy link
Copy Markdown
Member

gonna close this since #28104 supercedes this

story645 closed this Nov 27, 2024
melissawm moved this from Waiting for author to Done in First Time Contributors Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

[ENH]: Different edgecolor and hatch color in bar plot

4 participants


Back | FazBrowse Home | New Git URL