/matplotlib

Fix cycler validation #6275

Merged
merged 2 commits into from Apr 11, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  v2.0.0
4 participants
@WeatherGod @tacaswell @rathann @mdboom
@WeatherGod
Member

WeatherGod commented Apr 6, 2016

This should fix the cycler normalization/validation bugs we have seen such as #5875. This does require a minimum version of Cycler v0.9 to get the change_key() feature. We could go a few steps further and get full validation of property values for Cycler objects, but that will require Cycler v0.10, I think, so that I can introspect the cycler object better -- not out of the question, but I figure I would get this first step done first.

mdboom added the needs_review label Apr 6, 2016

@tacaswell
Owner

tacaswell commented Apr 8, 2016

@WeatherGod Why is there an extra merge commit in here?

@tacaswell tacaswell commented on an outdated diff Apr 8, 2016

View full outdated diff
lib/matplotlib/rcsetup.py
@@ -767,6 +759,17 @@ def validate_cycler(s):
else:
raise ValueError("object was not a string or Cycler instance: %s" % s)
+ unknowns = cycler_inst.keys - (set(_prop_validators.keys()) |
@tacaswell

tacaswell Apr 8, 2016

Owner

You do not need the keys as iterating over a dict gives the keys anyway.

@tacaswell tacaswell and 1 other commented on an outdated diff Apr 8, 2016

View full outdated diff
lib/matplotlib/rcsetup.py
@@ -767,6 +759,17 @@ def validate_cycler(s):
else:
raise ValueError("object was not a string or Cycler instance: %s" % s)
+ unknowns = cycler_inst.keys - (set(_prop_validators.keys()) |
+ set(_prop_aliases.keys()))
+ if unknowns:
+ raise ValueError("Unknown artist properties: %s" % unknowns)
+
+ # Not a full validation, but it'll at least normalize property names
+ # A fuller validation would require v0.10 of cycler.
+ for prop in cycler_inst.keys:
+ norm_prop = _prop_aliases.get(prop, prop)
+ cycler_inst.change_key(prop, norm_prop)
@tacaswell

tacaswell Apr 8, 2016

Owner

do we want to catch the ValueError that can come out of this and raise a better error?

This is a sneaky way to do double key validation 😜

@WeatherGod

WeatherGod Apr 8, 2016

Member

What would you suggest as a better error message? How about something along the lines of "cannot specify both '{}' and its alias '{}' in the same cycler"?

One concern I have is that this function can mutate a given cycler, and then raise an exception, which is not optimal.

@tacaswell

tacaswell Apr 8, 2016

Owner

maybe we should do this in two steps? One explicit check of no aliased keys and then one normalization step we know will work?

I like that error message

Worried I am (re)painting the bikeshed with this review.
@tacaswell
Owner

tacaswell commented Apr 8, 2016

👍 on doing the normalization here.

I suspect that this will not catch everything as aliases will still fall through the plot functions in funny ways.

tacaswell added this to the 1.5.2 (Critical bug fix release) milestone Apr 8, 2016

@WeatherGod WeatherGod Improvements to property cycler. Closes #5875 
* Allow key normalization to Cycler objects
* Validation is performed at more entry points
* Requires Cycler v0.9+
 bfe5e3f
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

This isn't meant to catch anything having to do with plotting functions. This is strictly about the problem arising from having multiple entry points from which users can supply a property cycler, and normalizing the aliases was only done at one of the entry points. Now, aliasing will happen at every entry point. We are still left with validation only happening if the cycler is passed as a string, but full validation will need the newer cycler features.
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

The extra merge commit seemed to have happened because I originally forked from the v1.5.x branch, but submitted this as against master instead. I'll fix that at the next batch of updates to this PR.
@tacaswell
Owner

tacaswell commented Apr 8, 2016

I mean that users can pass ls or linestyle, do we properly normalize that to sort out if we need to consult the property cycle or will we end up doing the double update of both ls and linestyle via the self.update(**kwargs) path?
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

That is the responsibility of the individual plotting functions. The plotting functions can now assume that the property cycle will always have the normalized property names. Previously, it was possible that it didn't always have that, and as such, would do a double-update. That is now fixed.

However, you can also get a double-update if the plotting function does not properly normalize its list of properties before consulting the property cycle. We are much better about this than we used to be, but still not perfect.
@tacaswell
Owner

tacaswell commented Apr 8, 2016

I think we are on the same page.
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

Of course, there is still the fun aspect of fall-back properties (e.g., using color when facecolor or linecolor isn't provided), and the weird mixing of terminology (e.g., eliding 'color' to mean 'facecolor' in bar()).
@WeatherGod WeatherGod Make the validation mutation-safe, and improve error messages

Some checks were not successful

1 failing and 3 successful checks
continuous-integration/appveyor/branch — AppVeyor build failed
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
2128c96
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

Please note, I accidentally pushed up this branch to matplotlib's repo. I deleted that branch, and cancelled the extra travis build that it triggered (comes up now as an error). I don't think I have any permissions to cancel the extra appveyor build, though.
@WeatherGod
Member

WeatherGod commented Apr 8, 2016

I just noticed that this is milestoned for v1.5.2. That might need to be reconsidered because of the version bump in the cycler dependency? Not that it is a difficult dependency, I just didn't know if we should do that in a bugfix release.

Also, it isn't like what I am fixing is a regression, since property cyclers were brand-new in v1.5.

WeatherGod closed this Apr 11, 2016

mdboom removed the needs_review label Apr 11, 2016

WeatherGod reopened this Apr 11, 2016

mdboom added the needs_review label Apr 11, 2016

@tacaswell tacaswell modified the milestone: 2.0 (style change major release), 1.5.2 (Critical bug fix release) Apr 11, 2016

@tacaswell
Owner

tacaswell commented Apr 11, 2016

I am sold on not bumping the version of required cycler on 1.5.x branch.
@WeatherGod
Member

WeatherGod commented Apr 11, 2016

cycling this PR didn't get rid of the bad Appveyor/Travis runs that happened when I accidentally pushed to the repo. So they should be ignored. The PR test runs are green.

@tacaswell tacaswell merged commit 6f167ca into matplotlib:master Apr 11, 2016
3 of 4 checks passed

3 of 4 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

tacaswell removed the needs_review label Apr 11, 2016

@tacaswell tacaswell added a commit that referenced this pull request Apr 12, 2016

@tacaswell tacaswell Merge pull request #6275 from WeatherGod/fix_cycler_validation 
Fix cycler validation
 337fb07
@tacaswell
Owner

tacaswell commented Apr 12, 2016

backported to v2.x as 337fb07

WeatherGod referenced this pull request Apr 27, 2016

Merged

BLD: make minimum cycler version 0.10.0 #6340

@rathann

rathann commented on bfe5e3f Aug 29, 2016

Actually, this requires cycler 0.10+, as it was implemented in 1898aa2, which is present only in 0.10.0 and master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment