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

[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792) by afvincent · Pull Request #9797 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792) #9797

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.

Sign up for GitHub

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

Closed
afvincent wants to merge 2 commits into from
Conversation 10 Commits 2 Checks 0 Files changed
Closed

[WiP] FIX/ENH: include (offset, on-off seq) as a valid pattern to the linestyle validator (fix #9792) #9797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
612d6d0
add offset-onoffseq pattern to ls validation
afvincent Nov 16, 2017
ffd3434
update the relevant test
afvincent Nov 16, 2017
File filter

Filter by extension

Filter by extension .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
22 changes: 19 additions & 3 deletions lib/matplotlib/rcsetup.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import warnings
import re

from matplotlib.cbook import mplDeprecation, deprecated, ls_mapper
from matplotlib.cbook import mplDeprecation, deprecated, ls_mapper, iterable
from matplotlib.fontconfig_pattern import parse_fontconfig_pattern
from matplotlib.colors import is_color_like

Expand Down Expand Up @@ -931,13 +931,29 @@ def _validate_linestyle(ls):
"sequence nor a valid string.".format(ls))

# Look for an on-off ink sequence (in points) *of even length*.
# Offset is set to None.
# Offset is set to None if not present.
try:
offset = None
# Look first for the case (offset, on-off seq). Take also care of the
# historically supported corner-case of an on-off seq like ["1", "2"].
if (len(ls) == 2 and iterable(ls[1]) and

Kojoley Nov 17, 2017

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

With len(ls) == 2 you implicitly support other than tuple containers (e.g. [None, [1.23, 456]]), if it is an expected behavior there is not test for it.

afvincent Nov 17, 2017

Copy link
Copy Markdown
Contributor Author

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

Well it is True that I did not realized how explicit the docstring was about “dash tuple” for (offset, on-off seq.). 🐑.

I guess that it means that the validator is actually a bit too loose and should be changed to accept only tuples for the case (offset, on-off seq.)... Or do we prefer to support non-tuple cases as well?

tacaswell Nov 19, 2017

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

In general we try to as forgiving as possible on inputs, I do not see a reason to be strict here.

afvincent Nov 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@tacaswell Well then should we change the docstrings that explicitly state "dash tuple" for custom line style argument (more or less everywhere :/)? Or is the currently implicit behavior fine? If we want to keep the valid pattern loose, I will at least add a few more tests with other than tuple containers, as @Kojoley suggested.
not isinstance(ls[0], six.string_types)):
# That is likely to be a pattern (offset, on-off seq), so extract
# the offset and continue with the on-off sequence candidate.
if ls[0] is not None:
try:
offset = float(ls[0])

tacaswell Nov 19, 2017

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

Do you need this assignment? I think this is just testing it can be cast to a float?

afvincent Nov 21, 2017

Copy link
Copy Markdown
Contributor Author

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

That is right. I'll see how to clean this when implementing @Kojoley's below suggestion too.
except ValueError:
raise ValueError(
"a linestyle offset should be None or "
"a numerical value, not {!r}.".format(ls[0]))
offset, ls = ls

Kojoley Nov 19, 2017

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

offset is overwritten here. Is not this line should be before if? (and you can replace ls[0] with offset in multiple places)

afvincent Nov 21, 2017

Copy link
Copy Markdown
Contributor Author

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

You're right... And indeed it seems that one can also factor a bit. I'll correct this for the next iteration.

if len(ls) % 2 != 0:
raise ValueError("the linestyle sequence {!r} is not of even "
"length.".format(ls))

return (None, validate_nseq_float()(ls))
return (offset, validate_nseq_float()(ls))

except (ValueError, TypeError):
# TypeError can be raised inside the instance of validate_nseq_float,
Expand Down
10 changes: 7 additions & 3 deletions lib/matplotlib/tests/test_rcparams.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -375,14 +375,18 @@ def generate_validator_testcases(valid):
(['1.23', '4.56'], (None, [1.23, 4.56])),
([1.23, 456], (None, [1.23, 456.0])),
([1, 2, 3, 4], (None, [1.0, 2.0, 3.0, 4.0])),
((-1, [1.23, 456]), (-1, [1.23, 456.0])),
((None, [1.23, 456]), (None, [1.23, 456.0])),
((9.87, [1.23, 456]), (9.87, [1.23, 456.0])),
((np.float128(1), [1.23, 4]), (1.0, [1.23, 4.0])),
),
'fail': (('aardvark', ValueError), # not a valid string
('dotted'.encode('utf-16'), ValueError), # even on PY2
((None, [1, 2]), ValueError), # (offset, dashes) != OK
((0, [1, 2]), ValueError), # idem
((-1, [1, 2]), ValueError), # idem
([1, 2, 3], ValueError), # sequence with odd length
(1.23, ValueError), # not a sequence
(("a", [1, 2]), ValueError), # wrong explicit offset
((1, [1, 2, 3]), ValueError), # odd length sequence
(([1, 2], 1), ValueError), # inverted offset/onoff
)
}
# Add some cases of bytes arguments that Python 2 can convert silently:
Expand Down
Back | FazBrowse Home | New Git URL