Skip to content

relative line widths, marker sizes and marker edge widths #10244

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

Conversation

fkloosterman
Copy link
Contributor

@fkloosterman fkloosterman commented Jan 13, 2018

PR Summary

This PR relates to issue #10173

Added support for relative line width, marker size and marker edge width (e.g. 'x-large', 'xx-thin') and corresponding rcParams. Note: in rcParams, lines.linewidth and lines.markersize are taken as the reference for relative widths/sizes and cannot be relative themselves. Also, relative width for hatch.linewidth is not (yet) supported.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

…dth (e.g. 'x-large', 'xx-thin') and corresponding rcParams. Note: in rc, lines.linewidth and lines.markersize are taken as the reference for relative widths/sizes and cannot be relative themselves. Also, relative width for hatch.linewidth is not (yet) supported.
@jklymak
Copy link
Member

jklymak commented Jan 13, 2018

This looks like a nice start... Can you include a test? Not sure if you need an image test, maybe just create the lines and show that they have the correct width?

@anntzer
Copy link
Contributor

anntzer commented Jan 14, 2018

I think relative sizes ("2x", etc) are a reasonable feature to add (and agree that having the number 2 and the string "2" mean different things is not so great, regardless of how colors do...), but I'm not so certain about "named" relative sizes. The relative font size names come from the CSS spec (https://developer.mozilla.org/en-US/docs/Web/CSS/font-size); I guess the CSS equivalent here would be https://developer.mozilla.org/en-US/docs/Web/CSS/border-width which only defines "thin", "medium" and "thick". In general, I prefer not introducing too much mpl-specific "words" ("xx-thin", etc.).

@jklymak
Copy link
Member

jklymak commented Jan 14, 2018

TikZ: https://tex.stackexchange.com/questions/106742/increase-the-thickness-of-node-border-in-tikz/106743#106743

\tikzset{
    ultra thin/.style= {line width=0.1pt},
    very thin/.style=  {line width=0.2pt},
    thin/.style=       {line width=0.4pt},% thin is the default
    semithick/.style=  {line width=0.6pt},
    thick/.style=      {line width=0.8pt},
    very thick/.style= {line width=1.2pt},
    ultra thick/.style={line width=1.6pt}
}

not to say I particularly like those names. Personally prefer x- and xx-. I (mildly) don't like "thicker" and "thinner", just because they aren't clearly on one side of "thick" and "thin".

I'm fine with '0.5x' etc.

Improved validators in rcsetup.py
rcsetup.cycler now uses new line width/marker size validators
Updated tests
@fkloosterman
Copy link
Contributor Author

I updated the PR so that qualitative size names and fractional size strings (e.g. '1.5x' or '150%') work. It will be easy enough to change what relative sizes are accepted, if so desired. I am in favor of both qualitative names and fractional size strings, but at least the fractional sizes should be supported.

I put some utility code in cbook.init so that rcsetup.py doesn't have to import lines.py - is cbook the right place to put common utilities?

Two validators in rcsetup.py are used to check all line width and marker size rc parameters to support absolute and relative widths/sizes. Exceptions are lines.linewidth and lines.markersize, which are used as reference for relative sizes and can only be absolute themselves. Another exception is hatch.linewidth, which is directly used in the backends (i.e. in backend_bases.py and in the ps and svg backends) and I am not sure how to handle this properly.

The property cycler defined in rcsetup.py now also uses the new line width and marker size validators.

Two questions:

  1. With the new code, the axisartist/simple_axisline3.py example appears to fail, apparently because the Ticks.__init__ constructor in mpl_toolkits/axisartist/axis_artist.py uses 'auto' as a default for markeredgewidth instead of None. With the new validation this stopped working. Should 'auto' be accepted as a value and treated the same as None or should the Ticks.__init__ method be updated to use None?
  2. I am still on the fence regarding the use of lines.linewidth and lines.markersize rc parameters as reference for relative sizes. Consider the following example:
    import matplotlib
    import matplotlib.pyplot as plt
    
    matplotlib.rcParams['axes.linewidth']='0.5x'
    
    with matplotlib.rc_context(rc={'lines.linewidth':2}):
        # the default axes line width has now changed to 0.5*2 = 1 point
        # which is probably not what the user wants
        fig, ax = plt.subplots()
        ax.plot([0,1], [0,1])
    Instead, I suggest to have new entries in rcParams: reference.linewidth and reference.markersize. What do people think?

@efiring
Copy link
Member

efiring commented Jan 15, 2018

Given the ability to specify '1.5x' or '150%', what do we (and users) gain by having all the names--'xx-small', etc? It's less explicit, and more for the user to either remember or look up.

@jklymak
Copy link
Member

jklymak commented Jan 15, 2018

I think people like having good-looking defaults. We could eventually let users spec what those defaults are if they want to tweak them.

@efiring
Copy link
Member

efiring commented Jan 15, 2018

I don't understand what this has to do with defaults.

@efiring
Copy link
Member

efiring commented Jan 15, 2018

What I'm concerned about it ending up with ever more ways of doing things, hence more code, and more for people to struggle to remember. I guess if the names follow the existing font_scalings names it's not so bad--but it is unfortunate that they include the synonyms, 'smaller' being equivalent to 'small', etc. I would prefer to see those deprecated rather than propagated.

@anntzer
Copy link
Contributor

anntzer commented Jan 15, 2018

In fact, looking at @efiring's comment and based on the fact that we may even have to introduce yet another rcParam I wonder if this feature is really warranted: one can just as well use 3*rcParams["lines.linewidth"] as linewidth; if that's too long for you just define your favorite alias (your own global LW, or a getter lw = lambda: rcParams["lines.linewidth"]) then use 3*LW or 3*lw() which is arguably easier to understand than "3x" (as it should be easier for the untrained eye to figure out what is the reference value).

@jklymak
Copy link
Member

jklymak commented Jan 15, 2018

I agree that if there is not a "thick" etc set of keywords, this PR isn't that necessary. OTOH, its still kind of nice to be able to just type 1.1x.

Maybe a compromise is to re-consider this when the new python-syntax rcParams gets added (3.0, right @anntzer? (thoigh I can't find the Issue where you laid that out, but I'd lvoe to see that in 3.0). Then it'd be a lot easier to lard up the rcParams w/o needing a bunch of akward validators, etc.

@anntzer
Copy link
Contributor

anntzer commented Jan 15, 2018

3.0 is still the target... #9528 is the PR.
We're comparing "1.1x" and 1.1*lw() and the latter is really not that much longer (or 1.1*LW which has exactly as many characters)...

@jklymak
Copy link
Member

jklymak commented Jan 15, 2018

Yeah, except one is atuomatic, the other you have to create the constant/method

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Per comments above and @efiring's comments, I am against adding this feature as-is (may revisit this after the new rcsystem gets in).
As usual, feel free to dismiss this review if other devs reach a consensus that this should be added.

@jklymak
Copy link
Member

jklymak commented May 2, 2018

I think two core devs opposed to this and no countervailing argument merit closing this otoh I thought it was a reasonable idea. Thanks a lot @fkloosterman!

@jklymak jklymak closed this May 2, 2018
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.

4 participants