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

Make it possible to add mpl.rcParams to itself or deepcopy #2555

Merged
merged 1 commit into from Jan 27, 2014

Conversation

jankatins
Copy link
Contributor

Before it was not possible to set all rcParams to mpl.rcParams or use
copy.deepcopy on mpl.rcParams.

There were two problems:

  1. validate_bool_maybe_none(None) was raising ValueError
    -> fixed by accepting this value
  2. svg.embed_char_paths was deprecated by svg.fonttype but both have
    different valid values (bool vs strings).
    -> fixed by adding a "value translation" lambda to _deprecated_map
    which translates the boolean values to the string values of
    svg.fonttype.

[EDIT]
Also changed all validators, which accept None to check strings
without case ("none" vs "None").

removed rcPrams.update method because there were too many warnings when using rc_context()
[/EDIT]

Also added a new test for this: mpl.tests.test_rcparams.test_Bug_2543()

I wasn't able to compile matplotlib under my windows environment and only did the changes in my original installed mathplot and the test in a ipyhton notebook. So please check and test before merging...

Closes: #2543

@jankatins
Copy link
Contributor Author

I think it would actually make sense to add and constructor to RcParams which would remove all deprecated params. The validators would still be there so they could be added, but they wouldn't be there if someone pokes into mpl.rcParams.

@jankatins
Copy link
Contributor Author

also: 'tk.pythoninspect' should probably be added to _deprecated_ignore_map but then the getter should be changed to always return None for such ignored params?

@jankatins
Copy link
Contributor Author

Ok, two build failues:

On 2.6

======================================================================
ERROR: matplotlib.tests.test_rcparams.test_Bug_2543
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/matplotlib-1.4.x-py2.6-linux-x86_64.egg/matplotlib/tests/test_rcparams.py", line 113, in test_Bug_2543
    with assert_raises(ValueError):
TypeError: failUnlessRaises() takes at least 3 arguments (2 given)

-> assert_raises(9 can only be used on 2.7+ -> http://stackoverflow.com/questions/17585207/python-unittest-assetraises

and on 3.x:

======================================================================
ERROR: matplotlib.tests.test_rcparams.test_Bug_2543
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/__init__.py", line 805, in __setitem__
    cval = self.validate[key](val)
  File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/rcsetup.py", line 60, in __call__
    % (self.key, s, list(six.itervalues(self.valid))))
ValueError: Unrecognized svg.fonttype string "True": valid strings are ['path', 'none', 'svgfont']
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/tests/test_rcparams.py", line 119, in test_Bug_2543
    mpl.rcParams['svg.fonttype'] = True
  File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/__init__.py", line 807, in __setitem__
    raise ValueError("Key %s: %s" % (key, ve.message))
AttributeError: 'ValueError' object has no attribute 'message'

@jankatins
Copy link
Contributor Author

Added changes to let the tests pass:

  • do not run the tests with context manager on 2.6
  • use str(ValueError) instead of ValueError.message

Before it was not possible to set all rcParams to mpl.rcParams or use
 copy.deepcopy on mpl.rcParams.

There were two problems:
 1. validate_bool_maybe_none(None) was raising ValueError
 -> fixed by accepting this value
 2. svg.embed_char_paths was deprecated by svg.fonttype but both have
 different valid values (bool vs strings).
 -> fixed by adding a "value translation" lambda to deprecatedmap
 which translates the boolean values to the string values of
 svg.fonttype.

Also changed all validators, which accept None to check strings
without case ("none" vs "None").

Test for this: mpl.tests.test_rcparams.test_Bug_2543()

Closes: matplotlib#2543
@mdboom
Copy link
Member

mdboom commented Nov 14, 2013

What's the use case for this? If it's to create a temporary context for parameters, would it be better to use rc_context?

@jankatins
Copy link
Contributor Author

I had to work around this when I implemented theming for yhat/ggpy#75. One of the problems was that theme_xkcd() is implemented by copying the complete mpl.rcParams structure to a dict and set that during plotting. See here:
https://github.com/JanSchulz/ggplot/blob/71124e0e8ec3156584302ac84fd97fe57d185f7e/ggplot/ggplot.py#L126 and https://github.com/JanSchulz/ggplot/blob/0b088fff3592fd2b25d3e8723c3bc2189be66927/ggplot/themes/theme_xkcd.py. I found that better than to copying/duplicating the code from the xkcd() method or implementing another callback. Setting None to certain rcParams resulted in an error and just using mpl.rcParam.update(new) resulted in problems because the values in the dict are not validated, so setting e.g. figsize to "3,4" results in an error during plotting (this would be the same as in with rc_context(rc={"figsize":"3,4"}):)

In another implementation idea I tried to copy the rcParam dict directly (without copying it to a dict), but that resulted in the problem with deepcopy when adding another geom/... to the ggplot object.

@jankatins
Copy link
Contributor Author

I found it very strange, that the deprecated keys are still added to rcParams. Wouldn't it make sense to "forget" deprecated keys in the default rcParams dict?

Edit: The obsolete defaults are in rcsetup.py -> defaultParams. They get added to each rcParams:

        ret = RcParams([(key, default) for key, (default, _) in \
                        six.iteritems(defaultParams)])

The easiest thing would be to add a __init__() to RcParams, which would call dict.__init__(...) and then remove every value in the deprecated maps. The result would be that you can still set and get values by these deprecated keys (due to the way the deprecated keys are replaced by their new values), but they would not anymore be included in the underlying dict and therefore not available during iteration or deepcopy.

@tacaswell
Copy link
Member

I think this should be merged, it fixes a bug.

I have restarted the tests to make sure they still pass.

@@ -92,6 +95,43 @@ def test_RcParams_class():
assert ['font.cursive', 'font.size'] == sorted(rc.find_all('i[vz]').keys())
assert ['font.family'] == list(six.iterkeys(rc.find_all('family')))

def test_Bug_2543():
Copy link
Member

Choose a reason for hiding this comment

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

I think this name could be better - as a reader I don't know what bug 5243 is, so describing the problem in the name would be a better approach IMHO.

mdboom added a commit that referenced this pull request Jan 27, 2014
Make it possible to add mpl.rcParams to itself or deepcopy
@mdboom mdboom merged commit 5079b6d into matplotlib:master Jan 27, 2014
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.

rcsetup.validate_bool_maybe_none(None) raises Exception
4 participants