Store dash_pattern as single attribute, not two. #21050
Conversation
lib/matplotlib/lines.py
Outdated
| # this is needed scaling the dash pattern by linewidth | ||
| self._us_dashSeq = None | ||
| self._us_dashOffset = 0 | ||
| self._us_dash_pattern = (0, None) # offset, dash (unscaled) |
There was a problem hiding this comment.
I‘m inclined to use a namedtuple instead of the comment for semantic clarity.
There was a problem hiding this comment.
At that point, should it be a dataclass so that it can eventually carry documentation/the linestyle docs can be pushed there?
There was a problem hiding this comment.
You could write self._dash_pattern = DashPattern(0, None) but I would argue that this is actually worse and still needs a comment (or DashPattern(offset=0, dashes=None) to not need a comment), as it is less clear that we're just taking two values and not doing anything fancy with them (whereas a constructor could do arbitrary things with its arguments).
Additionally, we'd never actually use the .offset and .dashes accessors, which removes the main benefit of having named fields (well, _scale_dashes could take a packed dashpattern rather than an unpacked dashpattern as parameter, but that doesn't seem like a big deal).
Using a dataclass would also makes things harder on the C (agg) side for parsing (well, it's always doable, but you get a bunch of PyObject_GetAttrString and PyFloat_AsDouble and error and refcount management and whatnot, versus PyArg_ParseTuple which does many things for you).
| @@ -603,8 +598,9 @@ def draw(self, renderer): | |||
| if not self.get_visible(): | |||
| return | |||
| # Patch has traditionally ignored the dashoffset. | |||
|
I'm not really sure this is better than just having two attributes? *-Expanding the tuple all the time doesn't improve readability. |
|
I think(?) storing as a single tuple makes it clearer that the two things are closely related and should basically always be considered together. |
|
This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details. To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease # assuming you are tracking your branchIf you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you. Thank you for your contributions to Matplotlib and sorry for the inconvenience. |
Dash offset and pattern are always used together, so just store them as single attributes on lines and patches, instead of separate ones.
Dash offset and pattern are always used together, so just store them as
single attributes on lines and patches, instead of separate ones.
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).