Skip to content
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

Enables setting hatch linewidth in Patches and Collections, also fixes setting hatch linewidth by rcParams #28048

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Impaler343
Copy link
Contributor

@Impaler343 Impaler343 commented Apr 9, 2024

PR summary

Fixes #21108 using the patch provided by @anntzer which I modified to pass tests. Need to write tests for:

  • Setting hatch linewidth in a Patch
  • Setting hatch linewidth in a Collection

PR checklist

@Impaler343
Copy link
Contributor Author

Impaler343 commented Apr 10, 2024

I don't know why docs and CI are failing, could somebody please check this out?

@rcomer
Copy link
Member

rcomer commented Apr 10, 2024

Hi @Impaler343, I'm not certain but I think the docs failure may be related to something that has been fixed on main. Could you try rebasing?

git fetch upstream
git rebase upstream/main

Appveyor is showing the same failure on all PRs right now, so I wouldn't worry about that one.

@Impaler343
Copy link
Contributor Author

Do I also need to squash?

lib/matplotlib/hatch.py Outdated Show resolved Hide resolved
Comment on lines +364 to +366
{linewidth:f} setlinewidth
{self._convert_path(
Path.hatch(hatch), Affine2D().scale(sidelen), simplify=False)}
Copy link
Member

Choose a reason for hiding this comment

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

Can you back these changes all the way out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes were done as flake8 failed while committing

Copy link
Member

Choose a reason for hiding this comment

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

flake8 should not have flagged either the g->f change or indentation in a multi-line string.

If flake8 is indeed triggering and requiring either of these changes that is a bug is flake8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've committing after skipping flake8, can't do anything else I think

@tacaswell
Copy link
Member

There is enough exploration/experimentation in here it is probably worth squashing to a handful of commits.

@tacaswell tacaswell added this to the v3.10.0 milestone Apr 10, 2024
@tacaswell
Copy link
Member

I think this is like 80% of the way there and looking pretty good!

@Impaler343
Copy link
Contributor Author

Impaler343 commented Apr 11, 2024

Can we get a feature for check_figures_equal() similar to image_comparison()'s remove_text feature? As the former is a more economical way to test, having it for check_figures_equal() would be a good idea. Or is it already available, and I don't see it?

@oscargus
Copy link
Contributor

Can we get a feature for check_figures_equal() similar to image_comparison()'s remove_text feature?

I do not think this is required? The reason remove_text exists is that the font files may be different on different machine. However, check_figures_equal() are executed on the same machine, so should never be a problem. Am I missing something?

@Impaler343
Copy link
Contributor Author

Impaler343 commented Apr 15, 2024

Can we get a feature for check_figures_equal() similar to image_comparison()'s remove_text feature?

I do not think this is required? The reason remove_text exists is that the font files may be different on different machine. However, check_figures_equal() are executed on the same machine, so should never be a problem. Am I missing something?

I suggested this as it is a quick plugin to make tests run faster by focussing on the important part of the plot, by removing axes, labels and ticks, and this most probably should make a significant difference while running a large amount of tests

return name

hatchPatterns = _api.deprecated("3.8")(property(lambda self: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hatchPatterns = _api.deprecated("3.8")(property(lambda self: {
hatchPatterns = _api.deprecated("3.10")(property(lambda self: {

An API change note is also required. See https://matplotlib.org/devdocs/devel/api_changes.html#deprecate-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please check if the API change note is written correctly? Also the failing test is unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Hatch linewidths cannot be modified in an rcParam context
5 participants