In principle, 👍. Some minor tweaks and a couple of trivial tests would push this over the line 😉

This seems to break saving animations and I have no idea why...

And it works if you run the test code in ipython but not when nose runs it...

The animation failures seem to be from the fact that a) the default values for animation.*_args were not valid values, but they don't get validated on the way into the default rcdict (that is the next thing to track down apparently).

When update is fixed to validate the input the old default value ('') was getting validated to [u''] which when that gets laundered through a list into extra_args in the animation code, the unvalidated values was extra_args = list(u'') which is an empty list, the validated value gave extra args = list([u'']) which is not an empty list. This resulted in there being an extra empty string being inserted in the command list passed to Popen which was causing everything to fail (it seemed to actually fail on frame 4 of streaming output....still no idea what to make of that, I assume it is how ever ffmpeg was dealing with that string and I don't want to understand).

I have fixed this two ways a) updated the default values and b) change the validate_stringlist function to drop empty strings while validating. I am not entirely sure that b) is the right thing to do in the case of a list/tuple passed in.

This patch also causes 3 warnings to be raised as they get validated on the update with the default step.

@pelson Can you take another look at this? I want to tag 1.4.1rc1 tonight.

gah, this seems to have now broken the build (due to unicode issues) and something strange with 2.6 and assert_raises.....

and I really should do something about the warnings on obsolete rcparams

Haven't looked at this in any great detail but it might be useful to look at 669c379 where I suppressed some warning of this type earlier. i.e. #3244

I think the better solution is to filter them out when creating the default rcparamdict (working on it)

I'm running home now, but there is sufficient uncertainty with this PR that is bothers me to merge and then immediately release a v1.4.1. What impact does the bug you're having fix? Is it worth holding this PR back for us to see a v1.4.1 through, and then apply this change subsequently (knowing that there is a real chance that we may not actually release a v1.4.2 and go straight to v1.5.0...).

@tacaswell - what do you think on the matter?

Without this fix pandas, networkx, and seaborn all can blow up with very cryptic errors using the macosx backend.

This affects enough of the code that I would rather get this into 1.4.1 and do a 1.4.2 if we have to when we find corner cases that didn't have test coverage rather than merge this later and then have it go out in 1.5.0 and have those same issues come up then after we have forgotten all of the details of this PR and will have a bunch of other hair-on-fire issues.

This bug has probably been there sense we did the sixification, but apparently no one uses the dev version + macosx + the styles in any of those packages.

@matplotlib/developers Can this get reviewed and get a consensus yes/no on if this should be in 1.4.1.

As I said above, I am in favor getting this in as soon as possible.

I agree; I think it makes sense to include it in 1.4.1. @pelson's factorization suggestion would be nice, but can always be done later.

Are we still stuck with the issue of individual characters getting through
to some of the keymap parameters instead of lists? I forget the issue
number, but it is a long-standing issue with no obvious solution.

On Sat, Oct 4, 2014 at 4:04 PM, Eric Firing notifications@github.com
wrote:

I agree; I think it makes sense to include it in 1.4.1. @pelson
https://github.com/pelson's factorization suggestion would be nice, but
can always be done later.

—
Reply to this email directly or view it on GitHub
#3564 (comment)
.

@WeatherGod This in fact fixes the problems with the keymap parameters

I agree; I think it makes sense to include it in 1.4.1

I'm actually falling on the other side of the fence - there will projects which depend on mpl which worked with v1.4.0 and not with v1.4.1 as a result of this change. I agree that they are making use of an API which we did not promise wouldn't break in the future, but the fact remains that there will be projects which are setting values which make zero difference to the resulting figure yet are not valid rcParameters. As a result of this change, the code will no longer work - I'm OK with forcing that issue, but not in a patch release (from v1.4 -> v1.5 would be a far better option).

Let me make this concrete.

v1.4.0 (fine):

import matplotlib.pyplot as plt
plt.rcParams.update({'savefig.jpeg_quality': 'foobar'})
plt.plot(range(10))

v1.4.1 (errors with ValueError: Key savefig.jpeg_quality: Could not convert "foobar" to int ):

import matplotlib.pyplot as plt
plt.rcParams.update({'savefig.jpeg_quality': 'foobar'})
plt.plot(range(10))

Of course, the actual problems will be far more subtle (lists vs scalars being the obvious problems).

have those same issues come up then after we have forgotten all of the details of this PR

The workaround would be to add warning to the update method if there are bad rcParameters for the intermediate v1.4.1 release.

Without this fix pandas, networkx, and seaborn all can blow up with very cryptic errors using the macosx backend.

What I'm lacking is a concrete example where you found this problem. Is this a seaborn rcParam which is being set, or is it a case of users updating the rcParam dict incorrectly? If the former, then are we instantly going to break old versions of, for example, seaborn when using mpl 1.4.1?

When I get back to ny I will wrap the up date and init validation in try except blocks + warnings.

What I don't see is a use case where a value in the rcparams fails the validation, but works correctly when used.

I see this a making the api do what we always claimed it did, not changing the api.

This is a tough call. I agree with @tacaswell that all we're doing is enforcing an API that was already there, not changing the API.

However, as this isn't really fixing a regression since 1.3 and it's so pervasive, maybe we'd better, as @pelson suggests, warn for now, and throw exceptions in 1.5...

@pelson example now runs, but prints a warning.

In [5]: /home/tcaswell/other_source/matplotlib/lib/matplotlib/__init__.py:892: UserWarning: Trying to set 'savefig.jpeg_quality' to 'foobar' via the update method of RcParams which does not validate cleanly. This warning will turn into an Exception in 1.5. If you think 'foobar' should validate correctly for rcParams['savefig.jpeg_quality'] please create an issue on github.
  func='update'))

In [6]: plt.rcParams['savefig.jpeg_quality']
Out[7]: 'foobar'

@pelson @mdboom Are you happy with this? I think this is the last blocker for 1.4.1.

Probably one last thing to do is double-check the keymap. This isn't covered in the unit tests.

@WeatherGod Added test.

Looks solid and all comments above have been addressed so I will merge this now.

Sorry to not have replied on this. What I meant was to double-check that the keymap handling code still works. To actually open up a plot and press all of the keys, making sure they still work.

Just checked and they work (couldn't test cmd-w as i don't have a mac and I'm not sure what 'keymap.all_axes' does).

I suspect there is some code in that handling which can be simplified, but I don't really want to touch that right now.

Good enough for me! Thanks!

On Tue, Oct 14, 2014 at 11:39 AM, Thomas A Caswell <notifications@github.com

wrote:

Just checked and they work (couldn't test cmd-w as i don't have a mac and
I'm not sure what 'keymap.all_axes' does).

I suspect there is some code in that handling which can be simplified, but
I don't really want to touch that right now.

—
Reply to this email directly or view it on GitHub
#3564 (comment)
.

Oh, and as for fixing up the keymap handling, after I finish my book, I will probably put together a MEP for a vastly improved keymap system that would allow users to extend its functionality (along with other features).