Skip to content

Update matplotlibrc.template #10531

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

Merged
merged 10 commits into from
Feb 26, 2018
Merged

Conversation

ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Feb 19, 2018

To fix #10494:

  • Update matplotlib.template with all currently supported rcParams
  • create tests to verify if
    • all rcParams are in the template file
    • the uncommented template is a valid rc file

PR Checklist

  • Updated rcParams
  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • Find out why codecov/project/tests doesn't like it

@ImportanceOfBeingErnest
Copy link
Member Author

ImportanceOfBeingErnest commented Feb 19, 2018

Some clarification:

RcParams

  • There are parameters verbose.level and verbose.fileio which are inside mpl.rcParams. It seems those are not actually used anymore or at least deprecated. However, they are not part of mpl._all_deprecated. I left them out of the template and the test, but should the actually be added to the mpl._all_deprecated?

  • There is a docstring.hardcopy : False in the file. When uncommenting, this will raise a warning. I have no idea what this parameter would even do. For the moment I left it in the template file, but commented with ## such that tests run smoothly.

  • The following parameters are inside of mpl.rcParams, so I added them to the template despite me not having any good explanation for them:

    pdf.use14corefonts : False
    pdf.inheritcolor : False
    pgf.rcfonts : True
    pgf.preamble : []
    pgf.texsystem : xelatex
    pgf.debug : False
    text.latex.preview : False
    animation.embed_limit : 20.0
    path.effects : []
    

    This might still be done once someone feels like they know a good comment string to add.

Tests

I added two tests:

  • Test if all rcParams are in the template file This should raise in case there is an rcParam in the mpl.defaultParams, which is not found in the template file. All params in mpl._all_deprecated and
    mpl._deprecated_set are excluded from the test. Also all params starting with _ and those with "verbose" (see first item). This test will hopefully force people adding new rcParams not to forget to add them to the template file as well.
  • Test if the uncommented template is a valid rc file. To test this, the format of the template file has been changed slightly. All rcParams are are commented with a single #, while true comments are prepended ##. This allows to read in that file, remove single leading # and read it in as rcparam file. This is done via mpl.rc_params_from_file. This would raise a Warning for every invalid line in the file. Since fail_on_error=True does not raise an error but only a warning, this test will probably never fail, but raise a bunch of warnings. Please comment on whether this is the desired way of performing such test.

@ImportanceOfBeingErnest
Copy link
Member Author

What does codecov/project/tests — 98.35% (-0.04%) compared to 56a6b9b failing mean?

@dstansby dstansby modified the milestones: v3.0, v2.2.0 Feb 19, 2018
@tacaswell
Copy link
Member

pytest has a context manager to capture warnings, I suggest using that and then asserting that there are no warnings.

The coverage report says that the coverage on the test suite has gone down, are you sure the new tests are actually running (pytest is always a bit mysterious to me)?

@ImportanceOfBeingErnest
Copy link
Member Author

  1. Ok, I changed it to raise an error if any warning happens.

  2. No, I'm not sure about anything here. ;-)
    What is "coverage"? How would one find out the test is run? I was under the impression that any function starting with test_ would be run?

@WeatherGod
Copy link
Member

WeatherGod commented Feb 19, 2018 via email

@tacaswell
Copy link
Member

I tend to fall back to putting assert False in locally and making sure the tests fail when I expect them to.

@ImportanceOfBeingErnest
Copy link
Member Author

My test only covers 0.01% of the all the code, simply because it is only written to test one specific thing. I think I'm misunderstanding something fundamental here.

Where exactly do you want me to put assert False and for what reason? I have made sure the test does what I expect it to do locally on my computer, is that what you mean?

So if there is anything you want me to do for this to successfully "cover" something or to be run at all, just tell me what it is. Feel free to take your time and do so whenever you're less busy since rushed one-sentence answers do not usually help much in such cases.

@WeatherGod
Copy link
Member

WeatherGod commented Feb 19, 2018 via email

newlines.append(newline)
#print(os.path.dirname(__file__))
fname = os.path.join(os.path.dirname(__file__),
'testrcvalid.temp')
Copy link
Member

Choose a reason for hiding this comment

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

pytest has a tempfile fixture that would be better to use for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

What exactly is the problem with that? Do I not have permission to create a file? Using a fixture to create a temporary directory and create a file inside seems rather overkill.

@WeatherGod
Copy link
Member

I am still not exactly sure how the test coverage has gone down. Is the size of the codebase including the test suite itself?

@ImportanceOfBeingErnest
Copy link
Member Author

The tests with assert False both failed as expected with an AssertionError on travis. So that means that they are indeed run.

@WeatherGod
Copy link
Member

WeatherGod commented Feb 19, 2018 via email

# This tests if the matplotlibrc.template file would result in a valid
# rc file if all lines are uncommented.
path_to_rc = mpl.matplotlib_fname()
path_to_rc = "matplotlibrc.txt" #mpl.matplotlib_fname() #
Copy link
Member

Choose a reason for hiding this comment

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

Was this left in here by accident?

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Please squash-merge this.

@WeatherGod
Copy link
Member

Should probably also clean out the debugging prints and other commented out things. But I am, in principle, +1 on this once we confirm that everything is working.

@ImportanceOfBeingErnest
Copy link
Member Author

The commented statements that are still left in the code are there by pupose: I was thinking that if someone has this test failing for his commit they would immediately direct him to the source of the problem.
Of course I might delete if you want.

if not found:
missing.update({k:v})
if missing:
raise ValueError("The following params are missing " +
Copy link
Member

Choose a reason for hiding this comment

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

assert not missing would be more inline with pytest conventions

assert missing == {} would also work.

Both should provide a reasonably informative error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I'm missing something, but assert not missing would show exactly as assert not missing in the Error, without any details, right? Here I would want to show all missing parameters in the error. That should make it relatively easy to just copy and paste them to the file.

Copy link
Member

@phobson phobson Feb 20, 2018

Choose a reason for hiding this comment

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

pytest is pretty good about reporting. Consider the following:

$ cat test_simple.py
import pytest

@pytest.fixture
def missing():
    return {'a': 1, 'b': 2}

def test_one(missing):
    assert not missing

def test_two(missing):
    assert missing == {}

So then running it:

============================== test session starts ===============================
platform linux -- Python 3.6.4, pytest-3.4.0, py-1.5.2, pluggy-0.6.0
Matplotlib: 2.1.2
Freetype: 2.8.1
rootdir: /mnt/c/Users/phobson/sources/scratch/simple_test, inifile:
plugins: pep8-1.0.6, mpl-0.9, cov-2.5.1
collected 2 items

test_simple.py FF                                                          [100%]

==================================== FAILURES ====================================
____________________________________ test_one ____________________________________

missing = {'a': 1, 'b': 2}

    def test_one(missing):
>       assert not missing
E       AssertionError: assert not {'a': 1, 'b': 2}

test_simple.py:9: AssertionError
____________________________________ test_two ____________________________________

missing = {'a': 1, 'b': 2}

    def test_two(missing):
>       assert missing == {}
E       AssertionError: assert {'a': 1, 'b': 2} == {}
E         Left contains more items:
E         {'a': 1, 'b': 2}
E         Use -v to get the full diff

test_simple.py:13: AssertionError
============================ 2 failed in 0.28 seconds ============================

It's pretty clear what's in the dictionary that should be empty

@WeatherGod
Copy link
Member

WeatherGod commented Feb 20, 2018 via email

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I think that if you go through the template and remove all the quote marks, it will be OK.

#animation.convert_path: 'convert' ## Path to ImageMagick's convert binary.
## On Windows use the full path since convert
## is also the name of a system tool.
#animation.convert_args: '' ## Additional arguments to pass to convert
Copy link
Member

Choose a reason for hiding this comment

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

Here and in all cases above, there should be no quotes for strings. A blank field is an empty string.

#keymap.zoom : o ## zoom mnemonic
#keymap.save : s ## saving current figure
#keymap.quit : ctrl+w, cmd+w ## close the current figure
#keymap.quit_all : 'W', 'cmd+W', 'Q' ## close all figures
Copy link
Member

Choose a reason for hiding this comment

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

There should be no quotes.

@WeatherGod
Copy link
Member

WeatherGod commented Feb 20, 2018 via email

@efiring
Copy link
Member

efiring commented Feb 20, 2018

That needs a big fat comment in the matplotlibrc.template, then, right before the prop_cycle line, pointing out that it is exceptional, and is the only place in the file where quotes are allowed, and that they are indeed required there.

@ImportanceOfBeingErnest
Copy link
Member Author

Thanks @efiring for coming back to the issue of string escapes. I brought his up here, but did not get any feedback, so I assumed this to be ok, as seen in #10421.

So, to make this accurate now: I removed all '' in the template, except for the cycler. I then ran a comparisson between the loaded rcParams and the defaultParams.

    rc = mpl.rc_params_from_file(fname,
                                     fail_on_error=True,
                                     use_default_template=False)
   
    d1 = dict(rc.items())
    d2 = dict(mpl.defaultParams.items())
    for k,v in d1.items():
        if d2[k][0] != v:
            print k
            print " ", d2[k][0], v
            print " ", type(d2[k][0]), type(v)

This brought up a number of parameters which did not match. I corrected those which needed to be updated or were obvious to fix. What remains is the following. Here the first item is the one from the defaultParams, the second is the one as read in from the template file. I'm trying to comment on each of them and would be happy about any feedback or action to take.

  • keymap.fullscreen
    ('f', 'ctrl+f') ['f', 'ctrl+f']
    <type 'tuple'> <type 'list'>
    All other keymaps seem to stored as list, except this one.

  • pgf.preamble
    [''] []
    <type 'list'> <type 'list'>
    A list with an empty string cannot be created from within the rc file syntax, or can it? If so how?

  • path.effects
    [] []
    <type 'list'> <type 'unicode'>
    How to create an empty list instead of the string '[]'?

  • savefig.bbox
    standard None
    <type 'str'> <type 'NoneType'>
    Despite the file saying savefig.bbox : standard the parsing yields this as None.

  • axes.prop_cycle This shows up, simply because two different instances of cycler.Cycler are created.

  • backend
    Agg agg
    <type 'str'> <type 'str'>
    Even if the file says backend : Agg this is parsed as lower case, while the defaultParams have the upper case string stored.

  • text.latex.preamble
    [''] []
    <type 'list'> <type 'list'>
    A list with an empty string cannot be created from within the rc file syntax, or can it? Same as pgf.preamble.

  • path.simplify_threshold
    0.111111111111 0.111111111111
    <type 'float'> <type 'float'>
    I supposed the stored value is 1./9

  • svg.hashsalt
    None None
    <type 'NoneType'> <type 'unicode'>
    String svg.hashsalt : None is not parsed to None, but "None".

If those cases were not showing up, one could ideally add the comparisson above to the tests, making sure not only the keys are correct and the file is correctly read in, but also that the values are up to date and correct.

@WeatherGod
Copy link
Member

WeatherGod commented Feb 20, 2018 via email

@efiring
Copy link
Member

efiring commented Feb 20, 2018

Partial response: I suspect that an empty list and a list with a single empty string are identical in practice--as they are subsequently used--but this can be checked. If not, it might be that some slight tweaks to the validation functions in rcsetup.py would be in order to fix that, but I would be very cautious about this.
I will be surprised if the matplotlibrc parsing actually requires ugly and confusing things like an isolated comma. The intention was to make it easy to read and write, hence the design that does not use quotes for strings--or didn't, until cycler came along.

#patch.edgecolor : black # if forced, or patch is not filled
#patch.force_edgecolor : False # True to always use edgecolor
#patch.antialiased : True # render patches in antialiased (no jaggies)
#patch.edgecolor : k ## if forced, or patch is not filled
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to exchange the long color names if favor of the short ones? IMO the long names are easier to understand, especially for someone not familiar with the matplotlib color system.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to exactly the value used in defaultParams, which is "k" or "w" instead of "black" or "white", this makes it now easy to identify which params have non-matching values compared to the default as stated above. Should I change it back?

Copy link
Member

Choose a reason for hiding this comment

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

I see the benefit of using the exact same values. From a user point of view, I would suggest to change the defaultParams to the long form. However I'm not an expert on matplotlib.rcParams and don't know if that has any negative side effects. This should be judged by someone with more experience in this field. I'll open an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Discussion here #10550. If this would get resolved immediately, I suggest to change the code of this PR back. Otherwise it's ok to start with the short forms in the template and a PR on #10550 would change them back.

with open(path_to_rc, "r") as f:
rclines = f.readlines()
missing = {}
for k,v in mpl.defaultParams.items():
Copy link
Member

Choose a reason for hiding this comment

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

For better readability, I'd slightly prefer for key, val or for name, val because the for loop is quite long.

rclines = f.readlines()
missing = {}
for k,v in mpl.defaultParams.items():
if k[0] == "_":
Copy link
Member

Choose a reason for hiding this comment

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

Maybe join:

if k.startswith(' ') or k in deprecated or 'verbose' in k:
    continue

continue
if "verbose" in k:
continue
found = False
Copy link
Member

@timhoffm timhoffm Feb 20, 2018

Choose a reason for hiding this comment

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

This (and following lines) can be written more compact:

if not any(k in line for line in lines):
    missing[k] = v

Note, that k in line is a bit imprecise because it would also match values or comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. I think any better solution here would eventually lead to writing one single test and match the parsed file agains the defaultParams. Depends on if people think this should be done or not.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of "incrementally better", so I don't want to enforce a full parsing test. Your partial test is already a big improvement. Maybe you could add a short comment to clarify that this test is not expected to be 100%. And of course, use the compact form 😃.

@efiring efiring merged commit c01f862 into matplotlib:master Feb 26, 2018
lumberbot-app bot pushed a commit that referenced this pull request Feb 26, 2018
QuLogic added a commit that referenced this pull request Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keeping matplotlibrc.templace up to date
7 participants