Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add rcfile function (which loads rc params from a given file). #861

Merged
merged 13 commits into from Aug 19, 2012

Conversation

Projects
None yet
5 participants
Contributor

memmett commented May 10, 2012

The new 'rcfile' function allows the user to load rc parameters from an explicitly named file.

The rc_params function was split into two to facilitate this change.

Member

ivanov commented May 11, 2012

thanks for the contribution, @memmett - though I haven't had a chance to test it yet. I also think it might make sense to wrap this functionality in a context manager - since if someone isn't simply changing their default RC, they are probably going to be using a few different ones. it would be something like

with rc_context(fname):
    plt.plot()
    ...

and then at the end, the original rc settings are restored.

@ivanov ivanov commented on an outdated diff May 11, 2012

lib/matplotlib/__init__.py
@@ -892,6 +898,13 @@ def rcdefaults():
"""
rcParams.update(rcParamsDefault)
+def rcfile(fname):
@ivanov

ivanov May 11, 2012

Member

rename this to rc_file, to be PEP8 compliant and consistent with the other functions around it

Contributor

memmett commented May 11, 2012

@ivanov, thanks for the suggestions. I'll try creating a context manager...

Member

ivanov commented May 11, 2012

just to be more explicit, I think that both types of functionality have their uses, they complement each other nicely.

Member

ivanov commented May 11, 2012

I sent a note to the -devel list about this, and also pinging @tonysyu and @arsenovic since tonysyu/mpltools#2 is getting at this as well.

Add rc_context and rename rcfile to rc_file.
rc_context is a context manager that optionally accepts a file name:

with mpl.rc_context():
    mpl.rc('lines', linewidth=22)
    ...

or

with mpl.rc_context(fname):
    ...
Contributor

memmett commented May 11, 2012

@ivanov I added a context manager that optionally accepts a filename, and renamed rcfile to rc_file.

Member

ivanov commented May 11, 2012

@memmett this is shaping up nicely, can you add docstrings to rc_context? Also, some tests for the new functionality would be great.

thinking some more about this, it occurs to me that it maybe also make sense to have rc_context also accept a dictionary. So one could do

with rc_context( {'font.size' : 20} ):
    plt.plot()
    ...

that way one wouldn't have to do the kind of rc param saving that you're now doing in the context manager by hand, but also not be forced to go through the file system in order to make use of the context manager.

There's probably a clean separation to be made here, but I'm struggling to see it at the moment. Certainly checking the type of the argument that was passed would be one approach to move forward. It reminds me of a similar bifurcation that happens whenever you want a function to accept either a filename or a file object, and I never feel good about solving that in the naive manner.

Contributor

memmett commented May 26, 2012

@ivanov I finally got around to adding a docstring for the context manager and allowing the context manager to accept a dictionary of new settings too.

I guess we could use non-keyword arguments and test their type (isinstance(arg, str)) to either: load settings from a file in the case of a string, or update the rcParams dictionary in the case of a dictionary. I think this (keyword arguments as currently implemented in the pull request vs non-keyword arguments as described) is a matter of taste - so I'll leave that to you and the other devs to decide. I'll be happy to implement either technique - just let me know.

Owner

mdboom commented Jun 1, 2012

This looks great. I'd like to see a unit test -- at least for rc_context to ensure that params added by the context manager are reverted after the with statement.

Member

pelson commented Aug 11, 2012

@memmett: I would love to see this feature in v1.2 (which is going to be frozen on the 20th). I believe the action is with you to add some tests. If you need a hand with this, please let us know and we can try to provide some guidance.

Contributor

memmett commented Aug 11, 2012

@pelson Thanks for reminding me about this! I'm happy to add some tests - where do these belong?

Owner

efiring commented Aug 12, 2012

It looks to me like the thing to do is make a new test module: lib/matplotlib/tests/test_rcparams.py

@pelson pelson commented on an outdated diff Aug 12, 2012

lib/matplotlib/tests/test_rcparams.py
+ with mpl.rc_context(rc={'text.usetex': not usetex}):
+ assert mpl.rcParams['text.usetex'] == (not usetex)
+ assert mpl.rcParams['text.usetex'] == usetex
+
+ # test context given filename (mpl.rc sets linewdith to 33)
+ with mpl.rc_context(fname=fname):
+ assert mpl.rcParams['lines.linewidth'] == 33
+ assert mpl.rcParams['lines.linewidth'] == linewidth
+
+ # test context given filename and dictionary
+ with mpl.rc_context(fname=fname, rc={'lines.linewidth': 44}):
+ assert mpl.rcParams['lines.linewidth'] == 44
+ assert mpl.rcParams['lines.linewidth'] == linewidth
+
+ # test rc_file
+ mpl.rc_file(fname)
@pelson

pelson Aug 12, 2012

Member

This will have a global effect on the tests and will need to be undone at the end of execution. Perhaps:

try:
    mpl.rc_file(fname)
    assert mpl.rcParams['lines.linewidth'] == 33
finally:
    linewidth = mpl.rcParams['lines.linewidth']

@pelson pelson commented on an outdated diff Aug 12, 2012

lib/matplotlib/tests/mpl.rc
@@ -0,0 +1 @@
@pelson

pelson Aug 12, 2012

Member

I think it would be better to rename this file to something like test_rcparams.rc.

Additionally, it might be worth mentioning inside the file that this is only used in the test_rcparams.py tests.

@pelson pelson commented on the diff Aug 12, 2012

lib/matplotlib/tests/test_rcparams.py
@@ -0,0 +1,35 @@
@pelson

pelson Aug 12, 2012

Member

This file needs to be added to matplotlib/__init__.py for automatic test pickup (see http://matplotlib.sourceforge.net/devel/coding_guide.html#creating-a-new-module-in-matplotlib-tests)

@pelson pelson commented on an outdated diff Aug 12, 2012

lib/matplotlib/tests/test_rcparams.py
@@ -0,0 +1,35 @@
+import os
+
+import matplotlib as mpl
+mpl.rc('text', usetex=False)
+mpl.rc('lines', linewidth=22)
+
+fname = os.path.abspath(os.path.dirname(__file__)) + os.sep + 'mpl.rc'
@pelson

pelson Aug 12, 2012

Member

This line is fine, but I don't believe the abspath is necessary. Normally:

os.path.join(os.path.dirname(__file__), 'mpl.rc')

Would suffice.

Member

pelson commented Aug 12, 2012

Other than my comments, this looks good to me. +1

Member

pelson commented Aug 19, 2012

This is good to go, but cannot be merged automatically. This is probably due to a collision in lib/matplotlib/__init__.py which would be easy to fix. @memmett are you able to rebase, if not, I will merge manually.

Member

pelson commented Aug 19, 2012

Actually, we will need to add something to doc/users/whats_new.rst. Would you mind doing it?

Contributor

memmett commented Aug 19, 2012

Should I put a note in "new in matplotlib-1.2"?

Contributor

memmett commented Aug 19, 2012

Alrighty, hopefully my rebase worked!

Member

pelson commented Aug 19, 2012

@memmett: Great work. I really like this feature. Merging.

pelson added a commit that referenced this pull request Aug 19, 2012

Merge pull request #861 from memmett/master
Add rcfile function (which loads rc params from a given file) and associated context manager.

@pelson pelson merged commit 709c709 into matplotlib:master Aug 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment