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

Simplify _bind_draw_path_function. #23571

Merged
merged 1 commit into from Aug 19, 2022
Merged

Simplify _bind_draw_path_function. #23571

merged 1 commit into from Aug 19, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 6, 2022

It doesn't actually need to be a context manager, but can be a plain
function, with an easier to explain behavior.

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

# FIXME: dpi_cor is for the dpi-dependency of the linewidth. There
# could be room for improvement. Maybe _get_path_in_displaycoord could
# take a renderer argument, but get_path should be adapted too.
self._dpi_cor = renderer.points_to_pixels(1.)
Copy link
Member

Choose a reason for hiding this comment

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

So its OK now for self._dpi_cor to be changed permanently here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already set permanently before?

lib/matplotlib/patches.py Outdated Show resolved Hide resolved
It doesn't actually need to be a context manager, but can be a plain
function, with an easier to explain behavior.
Comment on lines +585 to +595
path = self.get_path()
transform = self.get_transform()
tpath = transform.transform_path_non_affine(path)
affine = transform.get_affine()
self._draw_paths_with_artist_properties(
renderer,
[(tpath, affine,
# Work around a bug in the PDF and SVG renderers, which
# do not draw the hatches if the facecolor is fully
# transparent, but do if it is None.
self._facecolor if self._facecolor[3] else None)])
Copy link
Member

Choose a reason for hiding this comment

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

Optional: What do you think of introducing

_DrawPathArgs = namedtuple('_DrawPathArgs', 'path, transform, rgb_face')

The main motivation is to name the tuple elements because the function signature
where we could look up their meaning is quite far away.

With this we could write:

Suggested change
path = self.get_path()
transform = self.get_transform()
tpath = transform.transform_path_non_affine(path)
affine = transform.get_affine()
self._draw_paths_with_artist_properties(
renderer,
[(tpath, affine,
# Work around a bug in the PDF and SVG renderers, which
# do not draw the hatches if the facecolor is fully
# transparent, but do if it is None.
self._facecolor if self._facecolor[3] else None)])
transform = self.get_transform()
self._draw_paths_with_artist_properties(
renderer,
[_DrawPathArgs(
path=transform.transform_path_non_affine(self.get_path),
transform=transform.get_affine(),
# Work around a bug in the PDF and SVG renderers, which
# do not draw the hatches if the facecolor is fully
# transparent, but do if it is None.
rgb_face=self._facecolor if self._facecolor[3] else None),
])

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 switched to using a dict (and **kwargs unpacking), which seems just as good here.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 19, 2022

Actually #23571 (comment) doesn't really work because the parameter names are not part of the backend API; indeed, patheffects renderers use different parameter names. So really these arguments are currently positional and I reverted to the previous (tuple-based) approach.
We could consider making the parameter names part of the API, but that would mean a deprecation dance around patheffects (which I don't really want to do right now) and could also cause breakage for third-party backends which can be quite annoying to check for (something like replacing calls to draw_path(**kwargs) by try: draw_path(*kwargs) except TypeError: warn(); draw_path(*args); note that checking the function signature may explicitly not work for backends written as extension modules).

@timhoffm
Copy link
Member

Ok, let's not overengineer this. You can self-merge when green.

@tacaswell tacaswell added this to the v3.6.0 milestone Aug 19, 2022
@tacaswell tacaswell merged commit 52e6a12 into matplotlib:main Aug 19, 2022
@anntzer anntzer deleted the dpwap branch August 19, 2022 21:57
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.

None yet

5 participants