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
Use symbolic operator names (moveto, lineto) in contour_manual example. #25570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take or leave my remark.
M = Path.MOVETO | ||
L = Path.LINETO | ||
kinds01 = [[M, L, L, L, M, L, L, L]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at it, I would go a bit more verbose:
M = Path.MOVETO | |
L = Path.LINETO | |
kinds01 = [[M, L, L, L, M, L, L, L]] | |
MOVE = Path.MOVETO | |
LINE = Path.LINETO | |
kinds01 = [[MOVE, LINE, LINE, LINE, MOVE, LINE, LINE, LINE]] |
or even
steps = [
(Path.MOVETO, [0, 0]),
(Path.LINETO, [3, 0]),
(Path.LINETO, [3, 3]),
(Path.LINETO, [0, 3]),
(Path.MOVETO, [1, 1]),
(Path.LINETO, [1, 2]),
(Path.LINETO, [2, 2]),
(Path.LINETO, [3, 1]),
]
fills, kinds = zip(*steps)
But the proposal is also ok if you don't want to do more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think long names really help readability here; the point is really to see that there's 1 M followed by 3 Ls, twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that some way to "visually" connect the code and the points would be nice, but I am not really sure the zip-thing is worth it. So may just keep it as is.
|
||
import matplotlib.cm as cm | ||
from matplotlib.contour import ContourSet | ||
import matplotlib.pyplot as plt | ||
from matplotlib.path import Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the import ordering was decided to keep plt separate. Also, this is not really in alphabetical order anymore (which lead me to check the pre-commit and it is indeed failing...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
It's clearer to directly use the names (or their one-letter abbreviations M/L, which are e.g. part of the svg standard) that using numeric values and then explaining the numeric values.
Let’s take it as is. |
It's clearer to directly use the names (or their one-letter abbreviations M/L, which are e.g. part of the svg standard) that using numeric values and then explaining the numeric values.
PR Summary
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst