Skip to content
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

Rcparam validation fix #3564

Merged
merged 19 commits into from Oct 14, 2014
Merged

Conversation

tacaswell
Copy link
Member

Fixes an imaginary bug, a real bug, and a bug discovered while fixing the real bug.

This should get tests before it gets merged.

Close matplotlib#3470

Over-ride `update` so that inputs are validated.
Simplify and relax the validation for sequences of floats and
ints.

 - unified logic
 - input is not restricted to coma-separated lists, list, and tuples.
   any length 2 sequence (like an array) will now work.
@tacaswell tacaswell added this to the v1.4.1 milestone Sep 25, 2014
@@ -849,6 +849,16 @@ def __getitem__(self, key):
key = alt
return dict.__getitem__(self, key)

# http://stackoverflow.com/questions/2390827/how-to-properly-subclass-dict-and-override-get-set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the comment is necessary (at all), but a test would be very welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment could be shortened to one or two lines, but noting why update needs to be overridden is helpful; it's not obvious.

@pelson
Copy link
Member

pelson commented Sep 25, 2014

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

@tacaswell
Copy link
Member Author

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

@tacaswell
Copy link
Member Author

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

@tacaswell
Copy link
Member Author

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.

Same as `update`, the default behavior of `dict.__init__` does
not call `__setitem__` so we have not been validating values passed
at creation time.

Update the tests to account for the validation.
 - assert raise on invalid update
 - assert raise on invalid init
@tacaswell
Copy link
Member Author

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

@tacaswell
Copy link
Member Author

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

@tacaswell
Copy link
Member Author

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

@jenshnielsen
Copy link
Member

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

@tacaswell
Copy link
Member Author

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

@pelson
Copy link
Member

pelson commented Oct 2, 2014

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?

@tacaswell
Copy link
Member Author

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.

@tacaswell
Copy link
Member Author

@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.

@efiring
Copy link
Member

efiring commented Oct 4, 2014

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.

@WeatherGod
Copy link
Member

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)
.

@tacaswell
Copy link
Member Author

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

The other fixes on this branch make this un-needed (as the default
values of are now properly validated and a single word string (as
separated by commas) are correctly converted into length 1 list) but
out of principle make the default value of things labeled an 'string
lists' should have default values which are lists of strings.
@pelson
Copy link
Member

pelson commented Oct 9, 2014

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?

@tacaswell
Copy link
Member Author

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.

@mdboom
Copy link
Member

mdboom commented Oct 9, 2014

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...

Catch exceptions raised due to failed validations, force
setting the (un validated!) value, and print a warning.
@tacaswell
Copy link
Member Author

@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'

Added known-fail to tests that expect rcparam __init__ and
update to raise exceptions on invalid input.
@tacaswell
Copy link
Member Author

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

@WeatherGod
Copy link
Member

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

@tacaswell
Copy link
Member Author

@WeatherGod Added test.

@jenshnielsen
Copy link
Member

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

jenshnielsen added a commit that referenced this pull request Oct 14, 2014
@jenshnielsen jenshnielsen merged commit 27bdd59 into matplotlib:v1.4.x Oct 14, 2014
@tacaswell tacaswell deleted the rcparam_validation_fix branch October 14, 2014 13:54
@WeatherGod
Copy link
Member

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.

@tacaswell
Copy link
Member Author

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.

@WeatherGod
Copy link
Member

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)
.

@WeatherGod
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants