Skip to content

Get default params from matplotlibrc.template. #14929

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 30, 2019

This makes matplotlibrc.template a valid style file representing the
default style (modulo things that styles do not control, such as the
backend), avoids having to duplicate the defaults in rcsetup, removes
the need to test that the default matplotlibrc is up to date (if not,
matplotlib fails to import), and makes it easier to fit lines within 79
characters in rcsetup.py.

The only tricky points are:

  • datapath is not really an rcparam anyways, so just hack __getitem__
    to always return the correct value.
  • the entry for path.effects was incorrect, as [] would be validated as
    the "[]" list which is not a valid list of patheffects. In fact
    this rcParam cannot be meaningfully set from matplotlibrc; one would
    need to do an eval() like for axes.prop_cycle but let's not get
    there... I changed the validator to validate_anylist, which at least
    works for the default of an empty list...
  • we need to be a bit more careful when constructing the global rcParams
    instance as well as rcParamsOrig, rcParamsDefault, to not resolve
    _auto_backend_sentinel too early.

One can check that the rcParams are unchanged by printing them out --
that catches a typo: two entries in font.fantasy should be "Impact",
"Western"; not "ImpactWestern".

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer force-pushed the rc branch 5 times, most recently from 326ec0e to 6f62844 Compare July 31, 2019 08:35
@timhoffm
Copy link
Member

timhoffm commented Aug 10, 2019

Some questions / notes:

  1. We now need to have matplotlibrc in data_path. I assume before a matplotlibrc was not required. This makes the installation more brittle.
  2. What happens if the default matplotlibrc is missing a value? Do we fail on that? I think we should - otherwise rcParams will be incomplete and we'll get problems later.
  3. What about the datapath parameter? This now hard-coded through getitem. Before I think users could have overwritten this. - Not sure how reasonable that would be, but technically hard-coding is an API change. If datapath is not a parameter anyways, should we deprecate it in favor of get_data_path()?
  4. Are other libraries / programs hooking into the rcParams mechanism? The changes in rcsetup.py would likely break other libraries trying to add parameters.

Semi-OT: I think the config system could need a general overhaul, with among other things a simple way of resetting to defaults, layering, a public interface to add params for downstream libs, inferred values, etc. I've started a bit thinking about these things, but it's a bit early to come up with a proposal. This would also address the duplicate defaults.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 10, 2019

We now need to have matplotlibrc in data_path. I assume before a matplotlibrc was not required. This makes the installation more brittle.

I think that's approximately the same as requiring matplotlib/__init__.py: if you don't do anything silly, the matplotlibrc will be there (or it should be considered a bug in our packaging scripts).

What happens if the default matplotlibrc is missing a value? Do we fail on that? I think we should - otherwise rcParams will be incomplete and we'll get problems later.

Yes, it will fail in

    defaultParams = rcsetup.defaultParams = {  # Left only for backcompat.
        # We want to resolve deprecated rcParams, but not backend...
        key: [(rcsetup._auto_backend_sentinel if key == "backend" else
               rcParamsDefault[key]),
              validator]
        for key, validator in rcsetup._validators.items()}

(rcParams[key] will throw).

What about the datapath parameter? This now hard-coded through getitem. Before I think users could have overwritten this. - Not sure how reasonable that would be, but technically hard-coding is an API change. If datapath is not a parameter anyways, should we deprecate it in favor of get_data_path()?

It basically makes no sense for the end user to change it; see discussion at #11241. I plan to deprecate it.

Are other libraries / programs hooking into the rcParams mechanism? The changes in rcsetup.py would likely break other libraries trying to add parameters.

AFAICT This PR does not break the public API; probably you can still even append rcParams and have things work, not that I would recommend that...

Semi-OT: I think the config system could need a general overhaul, with among other things a simple way of resetting to defaults, layering, a public interface to add params for downstream libs, inferred values, etc. I've started a bit thinking about these things, but it's a bit early to come up with a proposal. This would also address the duplicate defaults.

Indeed, see https://github.com/matplotlib/mplcairo#antialiasing for an example case.

@jklymak
Copy link
Member

jklymak commented Aug 10, 2019

So now most users will have everything uncommented in their Matplotlibrc? That seems difficult to manage. How will a user easily see when they have made a non default change? Or when you install the template for a user who doesn’t have one does it replace all the comment marks?

@anntzer anntzer force-pushed the rc branch 3 times, most recently from 6da81ee to 7888374 Compare August 10, 2019 15:43
@anntzer
Copy link
Contributor Author

anntzer commented Aug 10, 2019

Users should not modify the one that's in matplotlib/mpl-data (which matplotlibrc explicitly warns against anyways), they should just place their own matplotlibrc in ~/.config/matplotlib or ~/.matplotlib anyways (which we don't create upon install), and that file can just have whatever entries they need.

@jklymak
Copy link
Member

jklymak commented Aug 10, 2019

Mine is definitely from the template. Though you are correct that it wasn't put there. But the way I originally made it was to copy the old template and search for the key I wanted and uncommented it and changed it. This change would make that harder.

This makes matplotlibrc.template a valid style file representing the
default style (modulo things that styles do not control, such as the
backend), avoids having to duplicate the defaults in rcsetup, removes
the need to test that the default matplotlibrc is up to date (if not,
matplotlib fails to import), and makes it easier to fit lines within 79
characters in rcsetup.py.

The only tricky points are:
- datapath is not really an rcparam anyways, so just hack `__getitem__`
  to always return the correct value.
- the entry for path.effects was incorrect, as [] would be validated as
  the "[]" list which is not a valid list of patheffects.  In fact
  this rcParam cannot be meaningfully set from matplotlibrc; one would
  need to do an eval() like for axes.prop_cycle but let's not get
  there...  I changed the validator to validate_anylist, which at least
  works for the default of an empty list...
- we need to be a bit more careful when constructing the global rcParams
  instance as well as rcParamsOrig, rcParamsDefault, to not resolve
  _auto_backend_sentinel too early.

One can check that the rcParams are unchanged by printing them out --
that catches a typo: two entries in font.fantasy should be "Impact",
"Western"; not "ImpactWestern".
@anntzer
Copy link
Contributor Author

anntzer commented Aug 10, 2019

I would say that if you care about tracking which keys you've changed away from the default, it's easier to create a new file and just copy the lines that you want to change to the new file, rather than copying a file with hundreds of lines and uncommenting a few of them?

@ImportanceOfBeingErnest
Copy link
Member

I fear a bit that this is what would happen... People will copy that file, make the change they want to make and have all the other parameters activated in their customized rc file. This can lead to all kinds of problems, like style files not being compatible between versions, keeping deprecated parameters around or absolute paths to video converters being distributed as style files.
Also we would then have a situation where some parameters like backend are still commented; it's not very transparent why that would be the case.

My proposal would be as follows: If you want to have the parameters loaded from a dedicated file, it should not be the matplotlibrc.template. Instead one could have it inside another file, say default_params.rc or so, then upon building matplotlib, that file gets copied, and each line gets an additional # in front to restore the current template.
The drawback of this is that the repo itself wouldn't contain the actual template anymore. But perhaps one could make the function to restore the template public, so people can call it if they messed it up.

@jklymak
Copy link
Member

jklymak commented Aug 10, 2019

Yeah I think people will definitely copy the full file. For all they know there is some magic in there somewhere that needs to be in the file so caution says to copy the whole file.

@timhoffm
Copy link
Member

then upon building matplotlib, that file gets copied, and each line gets an additional # in front to restore the current template.
The drawback of this is that the repo itself wouldn't contain the actual template anymore.

Could also do it the other way round: Keep matplotlibrc.template and upon building copy to matplotlibrc.default and remove one # from every line.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 10, 2019

I guess I don't actually have a good guess as to where people are copying the full file from: from the repo's matplotlibrc.template? from https://matplotlib.org/tutorials/introductory/customizing.html#a-sample-matplotlibrc-file? from their install's site-packages/matplotlib/mpl-data/matplotlibrc? Because this affects where we should put the default style that gets picked up on import.
I guess another solution may be to keep the things commented out, but, at startup, lookup site-packages/matplotlib/mpl-data/matplotlibrc (which would be fully commented out) and load it with a special loader that fully uncomments it first...

@jklymak
Copy link
Member

jklymak commented Aug 10, 2019

That seems reasonable. Only thing bad there is that someone will edit that file in place at some point, but not sure what we can do about that.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 11, 2019

OK, see alternate implementation at #15029.

@tacaswell
Copy link
Member

I prefer #15029 to this one.

@tacaswell tacaswell added this to the unassigned milestone Aug 12, 2019
@timhoffm
Copy link
Member

Closing in favor of #15029.

@timhoffm timhoffm closed this Feb 16, 2020
@anntzer anntzer deleted the rc branch February 16, 2020 10:11
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.

5 participants