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

Add xkcd style file. #14943

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add xkcd style file. #14943

wants to merge 2 commits into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 31, 2019

PR Summary

Closes #5992; replaces #6157. I guess @WeatherGod may not like the approach here so just making sure he gets pinged...

xkcd-style can't be implemented as an actual matplotlibrc-syntax style file because we can't specify patheffects in these. Instead, load these as regular, good-old Python modules. Because this can execute arbitrary Python code (this is the part @WeatherGod may not like...), restrict this capability to style files shipped by Matplotlib itself (which are certainly safe).

Also use this to deduplicate a bunch of common entries in seaborn styles.

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

@WeatherGod
Copy link
Member

What would be more awesome and useful is to expand how we can specify path effects. Such a feature could be used in more places and would allow style files outside of the package to achieve awesome effects.

All the while, you now have two completely separate ways to load styles, and users wanting to have something similar to a packaged style can't just copy the packaged style and modify it because it won't work outside the package.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 31, 2019

All the while, you now have two completely separate ways to load styles, and users wanting to have something similar to a packaged style can't just copy the packaged style and modify it because it won't work outside the package.

Of course it will work: they can stick the py file wherever in their PYTHONPATH (or even, god forbid, publish that as a PyPI/conda package that people can pip/conda-install) and do import mystyle; matplotlib.use(mystyle.__mpl_style__) (again, name up to bikeshedding). In fact that's easier that distributing mplstyle files: how exactly do you propose to distribute an mplstyle file with a PyPI/conda package and have them end up at the right place? (See https://gitter.im/matplotlib/matplotlib?at=5d41908d5367476cc985a18e which is what led me to write this PR.)

What would be more awesome and useful is to expand how we can specify path effects. Such a feature could be used in more places and would allow style files outside of the package to achieve awesome effects.

The proposal here achieves the desired feature. (Well, we can always implement more path effects, but I doubt that's what you're talking about.)

Note that this is explicitly not about using this approach for the matplotlibrc file (#9528) which I know you are opposed to and which I am not particularly interested in relitigating here.

@WeatherGod
Copy link
Member

Well, we can always implement more path effects, but I doubt that's what you're talking about.

Don't know how "more path effects" came into this discussion. I explicitly said "expand how we can specify path effects". Right now, we don't have a uniform way to specify and parametrize the path effects that we have now. That is a deficiency in our current API. Trying to come up with ways around this deficiency in a restricted (albeit popular) use-case still doesn't address the fundamental problem of a deficient path effects API.

Part of the problem is with how path effects is currently implemented using AGG and so we are very restricted in how we can utilize path effects. These are real problems that needs solving. I would love to have a way to draw cold fronts and warm fronts on my maps using matplotlib path effects, or to be able to do sawtooth lines in certain places, but the patheffects feature is just that limited.

Note that this is explicitly not about using this approach for the matplotlibrc file (#9528) which I know you are opposed to and which I am not particularly interested in relitigating here.

Yes, I realize that, and this approach (at least in theory, I haven't dived deep into the code yet) would address my primary concern of potentially executing untrusted code from outside site-packages that came up in #9528.

What I am mildly concerned about here is that one could do matplotlib.style.use('seaborn-dark'), and they can do matplotlib.style.use('myneatstyle') if they have a config-style myneatstyle.mplstyle in one of their user library locations. But, if they go take seaborn-dark.mplstyle and copy it to myneatstyle.mplstyle, and then tweak it, perhaps not fully realizing that it is python (maybe they thought it was JSON, perhaps), the matplotlib.style.use('myneatstyle') command won't work.

I know the example is a bit contrived, but it is to serve the point that having two different kinds of mplstyle files, with one kind of them working one way, and another kind working in a different way is just sowing confusion for our users. And for what gain? The ability to specify the xkcd path effects in a style file?

@anntzer
Copy link
Contributor Author

anntzer commented Jul 31, 2019

Don't know how "more path effects" came into this discussion. I explicitly said "expand how we can specify path effects"

Sorry, I misread you there.

Right now, we don't have a uniform way to specify and parametrize the path effects that we have now. That is a deficiency in our current API. Trying to come up with ways around this deficiency in a restricted (albeit popular) use-case still doesn't address the fundamental problem of a deficient path effects API.

A concrete proposal would be nice. But note that at the end of the day, a patheffect is more or less an arbitrary transform of a path (not just its coordinates, but also its color, its linewidth, etc.) (well, it can be even more, but let's start with something simple) -- sure, you can design a DSL to describe such transforms -- or you can just say, hey, I already have such a description language, it's called Python.

Part of the problem is with how path effects is currently implemented using AGG and so we are very restricted in how we can utilize path effects. These are real problems that needs solving. I would love to have a way to draw cold fronts and warm fronts on my maps using matplotlib path effects, or to be able to do sawtooth lines in certain places, but the patheffects feature is just that limited.

Path effects have nothing to do with Agg (for example you can run misc/patheffects_demo.py and save the result in svg and get a fully vectorized patheffect). You may be confusing them with the Agg filter -- which also has nothing to do with Agg per se, but at least does require a rasterization step.

What I am mildly concerned about here is that one could do matplotlib.style.use('seaborn-dark'), and they can do matplotlib.style.use('myneatstyle') if they have a config-style myneatstyle.mplstyle in one of their user library locations. But, if they go take seaborn-dark.mplstyle and copy it to myneatstyle.mplstyle, and then tweak it, perhaps not fully realizing that it is python (maybe they thought it was JSON, perhaps), the matplotlib.style.use('myneatstyle') command won't work.

The file is called seaborn-dark.py. Obviously, people can believe any kinds of things, but hopefully, they will realize that a .py file is a python file. It certainly can't be json, as its contents are of the form

__mpl_style__ = {...}

I know the example is a bit contrived, but it is to serve the point that having two different kinds of mplstyle files, with one kind of them working one way, and another kind working in a different way is just sowing confusion for our users. And for what gain? The ability to specify the xkcd path effects in a style file?

You're the one who opened #5992, not me. (Although the ability to stop having xkcd as a special case and being able to treat it as a normal style does appeal to me.)

Also, if you really think that's too confusing, we could technically switch all builtin files to .py... which per the above are easier to distribute anyways for third-party style designers.

@WeatherGod
Copy link
Member

sure, you can design a DSL to describe such transforms

I actually considered doing so a few years ago. Basically, we would have a library of transforms and effects that we can specify in a semicolon delimited string. I got lost in trying to understand the path code, particularly the part that interacts with the Agg filter. It would be nice if I could ever get cycles to pick up that work again.

The file is called seaborn-dark.py.

Ah, yes, I misread the diff. The filename does end in .py, not .mplstyle. With respect to confusing it for JSON, I was thinking more along the lines of the equivalent .py file from default.mplstyle or classic.mplstyle where the dictionary would be so large that if a user focused in on a part of the file they might not have noticed the assignment at the top. But, like I said, now noticing that these files would have .py as the extension, not .mplstyle, the confusion would be significantly lessened. Although there is still the conceptual "hiccup" of some style files are config-like, and others are not. Like I said, it is a mild concern.

You're the one who opened #5992, not me.

Yes, I did. And it was in the course of that issue I realized that addressing that issue was going to be harder than just slapping together a style file. It also lead me down the fun rabbit hole of Agg filters (yes, I got that mixed up, sorry), and my free time got all eaten up on this. But also realize that this issue just wasn't that important to me. It would be nice, yes, but in the context of everything else that I need from this library, it is of miniscule importance.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 31, 2019

particularly the part that interacts with the Agg filter.

Again, path effects are completely independent of Agg or the (poorly named) Agg filter.

there is still the conceptual "hiccup" of some style files are config-like, and others are not.

We can make all of them .py files.

this issue just wasn't that important to me.

Do you want to just close #5992 then (wrt. xkcd specifically)?


What kind of approach do you think we should suggest to people who want to distribute their own style files (see linked gitter discussion above)?

@WeatherGod
Copy link
Member

Again, path effects are completely independent of Agg or the (poorly named) Agg filter.

I got ahead of myself. I am talking about an idea I had years ago to expand lib/src/path_converters.h so that the path generator could get modified in a variety of different ways, and I got lost trying to figure out how to utilize some of AGG's features to achieve some of the effects I wanted.

We can make all of them .py files.

That was sarcasm, right?

Do you want to just close #5992 then (wrt. xkcd specifically)?

You don't close an issue just because one doesn't have a solution for it. The root problem is still valid. The current API is still limited, requiring logic in some parts rather than being declarative. That should be our goal, and would improve the library overall, not just in the style files. We have made good headway over the past few years, but we still have work to do.

As for the distribution question, this PR doesn't really change anything in that regard. One can right now distribute a package that builds up a module-level dictionary upon import, and then the user can call matplotlib.style.use(mymodule.mplstyle).

@anntzer
Copy link
Contributor Author

anntzer commented Jul 31, 2019

That was sarcasm, right?

No. They are all builtin styles fully under our control, so there is no security issue with making them python files.

We have made good headway over the past few years, but we still have work to do.

I am genuinely curious about what kind of "good headway" happened since the introduction of xkcd styles (the only thing I can think of is #11558, though I may obviously have missed some).

The root problem is still valid.

With all due respect, I simply fail to understand what the "root problem" is. A concrete example, with a way to achieve the desired plot (even if the API is not optimal), would be appreciated. What kinds of path transforms do you want? How do you propose to implement them (regardless of API); or, at least, what other software packages implement such transforms?

Also, FWIW, this request is now actually quite far from the original issue, which is

Can't believe we don't have this one already. Not perfectly trivial, as we would need a way to specify the special patheffect from the rcfile, but, essentially, this would replace plt.xkcd().
Should also look into finding an open font that we can package that matches Humor Sans.

I do believe this specific PR handles this specific issue; if you are looking for something else consider opening another issue...

One can right now distribute a package that builds up a module-level dictionary upon import, and then the user can call matplotlib.style.use(mymodule.mplstyle).

So you agree that distributing mplstyle files (in matplotlibrc syntax) is not best?

@WeatherGod
Copy link
Member

No. They are all builtin styles fully under our control, so there is no security issue with making them python files.

I read your comment to mean "all" style files, not just the builtins. Reading it as just builtins, I don't see how that addresses my (mild) concern about two kinds of style files that can't be used interchangably.

I am genuinely curious about what kind of "good headway" happened since the introduction of xkcd styles (the only thing I can think of is #11558, though I may obviously have missed some).

Back around the time plt.xkcd() was added, the artist/collection property system was very problematic. A lot of problems were uncovered and addressed during the leadup to mpl 2.0, and more bugs have been solved since then for a more consistent and sane UX. In addition, more entries have been added to the style dictionary to control more things, which is also a plus.

I do believe this specific PR handles this specific issue; if you are looking for something else consider opening another issue..

This specific issue is merely a symptom of a deeper problem, and that is that it is currently necessary to set "path.effects" to [patheffects.withStroke(linewidth=4, foreground="w")], rather than allowing for some syntax that does not require executing arbitrary code.

A concrete example, with a way to achieve the desired plot (even if the API is not optimal), would be appreciated. What kinds of path transforms do you want? How do you propose to implement them (regardless of API); or, at least, what other software packages implement such transforms?

That is besides the point. The point is that even with our current featureset we don't have a way to fully specify from an rc file all the different kinds of patheffects we have available. Most everything else can be specified from a nice, unified API (colors, line styles, draw styles, hatches, sizes, etc.), but not this.

So you agree that distributing mplstyle files (in matplotlibrc syntax) is not best?

I made no such determination. My area of expertise isn't about how to distribute resources, so I haven't spent any time thinking about what is and isn't the best way to this. I was only stating that this PR doesn't really address that question since your preferred approach can be done right now without any changes to the existing code.

The only thing this changes is establishing some sort of standard mechanism for one to fetch a style that is completely separate from the existing mechanisms. From that context, this is an intriguing idea and has merit, I am just concerned about whether or not this feature will be confusing to users in light of the existing style loading features.

Aside from my concerns about possible user confusion, I do wonder if entry points might be a better approach, but I haven't had the cycles to think it through.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 31, 2019

I read your comment to mean "all" style files, not just the builtins. Reading it as just builtins, I don't see how that addresses my (mild) concern about two kinds of style files that can't be used interchangably.

My point is that we can change our files to be .py; if others want to follow that path, that's great too :) (but their security problems are theirs, not ours)

Back around the time plt.xkcd() was added, the artist/collection property system was very problematic. A lot of problems were uncovered and addressed during the leadup to mpl 2.0, and more bugs have been solved since then for a more consistent and sane UX. In addition, more entries have been added to the style dictionary to control more things, which is also a plus.

I thought you were specifically talking about the path effects system, which has seen little activity AFAIK; I obviously agree other parts of the library have progressed.

This specific issue is merely a symptom of a deeper problem, and that is that it is currently necessary to set "path.effects" to [patheffects.withStroke(linewidth=4, foreground="w")], rather than allowing for some syntax that does not require executing arbitrary code.

My point is that a path transform should be able to execute arbitrary code; if not, it will always be restricted to some combination of library transforms that we provide (isn't this statement tautological?).

That is besides the point. The point is that even with our current featureset we don't have a way to fully specify from an rc file all the different kinds of patheffects we have available. Most everything else can be specified from a nice, unified API (colors, line styles, draw styles, hatches, sizes, etc.), but not this.

I believe that if an issue has been opened for >3y, with not even a concrete proposal on how to fix it within the constraints imposed by the author ("pyparsing will probably work" is not a concrete proposal), it may be worth considering whether there is any benefit in leaving that issue opened.

I made no such determination. My area of expertise isn't about how to distribute resources, so I haven't spent any time thinking about what is and isn't the best way to this. I was only stating that this PR doesn't really address that question since your preferred approach can be done right now without any changes to the existing code.
The only thing this changes is establishing some sort of standard mechanism for one to fetch a style that is completely separate from the existing mechanisms. From that context, this is an intriguing idea and has merit, I am just concerned about whether or not this feature will be confusing to users in light of the existing style loading features.

As you mention yourself, both mechanisms already exist, so this is not establishing a new mechanism that's separate from existing mechanisms.

Aside from my concerns about possible user confusion, I do wonder if entry points might be a better approach, but I haven't had the cycles to think it through.

I guess they may work too, though I am curious what advantage you think they may have.

@WeatherGod
Copy link
Member

I have had the evening to step back a re-examine this PR. As it was initially framed, this seemed like a heavy-handed solution to a relatively minor annoyance (no xkcd style file). Indeed, the xkcd feature has always been intended as a "challenge" to matplotlib in order to expose its limitations.

But, setting all of that aside and re-examining this PR from the perspective of how to make distribution and sharing of styles easier, I think this is a fascinating idea. Some issues I see with the current approach.

  1. Discoverability. Right now, we can have matplotlib report back all of the available styles. With this approach, these packaged styles won't be included in that report, which may lead to confusion.

  2. Only one style per package. Seems to be a bit of an arbitrary limitation.

  3. Could we also use the same idea for colormaps?

  4. Import side-effects. I am a little concerned that users may not be totally aware that calling matplotlib.style.use('myneatstyle') might be triggering an import, which may have unintended side-effects. Not exactly sure what the practical implications might be, but I do wonder if there is an existing python feature to isolate an import somehow in order to limit its side-effects.

I am not totally up on entry points, but if I understand them correctly, it may be possible to address the discoverability question with packages registering their styles upon install.

Perhaps this idea needs to be fleshed out a bit first in a MEP, which could also be used to document (bikeshed) the mechanisms we want packages to adopt?

@anntzer
Copy link
Contributor Author

anntzer commented Aug 1, 2019

Discoverability. Right now, we can have matplotlib report back all of the available styles. With this approach, these packaged styles won't be included in that report, which may lead to confusion.

I don't really think being able to list all third-party styles in particularly useful -- you can't list all importable packages in python either (ok, you can, but it's not so easy). I'm not saying it has no use, just that it may not be worth jumping through a million hoops to achieve that. To give an example from matplotlib itself, we don't have a list of all available projections (3d, various cartopy things, various astropy things) (at least before importing the module that defined them) and I haven't seen much complaints about it. If we really care about that -- see below re: entrypoints.

Only one style per package. Seems to be a bit of an arbitrary limitation.

You can easily have multiple styles per package -- put them in individual submodules (or even multiple dicts in the same module, if we don't care about the __mpl_style__ convention).

Could we also use the same idea for colormaps?

Already exists, see #6033 (comment) (which can be made into a proper doc entry). I could easily generate a similar template for styles...

Import side-effects. I am a little concerned that users may not be totally aware that calling matplotlib.style.use('myneatstyle') might be triggering an import, which may have unintended side-effects. Not exactly sure what the practical implications might be, but I do wonder if there is an existing python feature to isolate an import somehow in order to limit its side-effects.

Sandboxing python is a lost cause (everyone tried to do it for 20y or so). I think third-party styles should not be usable without an explicit import anyways, i.e. one should write import foo; mpl.style.use(foo.__mpl_style__) (or import foo; mpl.style.use(foo) if we adopt the convention of looking up __mpl_style__).
I don't think an additional import is too onerous -- just like it is now universally(?) agreed that importing matplotlib and numpy separately is better than importing star from pylab.

I am not totally up on entry points, but if I understand them correctly, it may be possible to address the discoverability question with packages registering their styles upon install.

Indeed, but entrypoint enumeration is slow (and it's a price that would be paid at each import). A similar idea for registering projections has been shot down some time ago on that argument. You may also want to read https://packaging.python.org/guides/creating-and-discovering-plugins/ which discusses the available options.

Perhaps this idea needs to be fleshed out a bit first in a MEP, which could also be used to document (bikeshed) the mechanisms we want packages to adopt?

Do you mean specifically the ability to enumerate styles?

@tacaswell tacaswell added this to the v3.3.0 milestone Aug 11, 2019
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@jklymak jklymak mentioned this pull request Jul 14, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021
@jklymak jklymak marked this pull request as draft May 8, 2021 20:34
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 18, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 26, 2023

Rebased, per #5992 (comment).

@anntzer anntzer removed the status: inactive Marked by the “Stale” Github Action label Jun 26, 2023
@anntzer anntzer marked this pull request as ready for review June 26, 2023 16:49


__mpl_style__ = {
**ast.literal_eval(Path(__file__).with_name("_seaborn-v0_8-common.py").read_text()),
Copy link
Member

Choose a reason for hiding this comment

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

This is what I'm dreading about python style files: Hard-to-follow dynamically defined contents.

IMHO we should use algorithmic code structures very sparingly in style files. There's great value in a declarative definition.

Furthermore, I assume that that this is done to prevent repetition and enforce consistency (DRY). However, style files are static (in fact the only time we have changed these ever was renaming them once). So, getting accidentally inconsistent is not a practical problem.

Let's just keep the pattern from the .mplstyle file of writing everything out and marking the common and individual parts by comments.

Copy link
Member

Choose a reason for hiding this comment

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

Can we not just make the common file normal Python and import it, instead of replicating all that with our own evaluation stuff?

Copy link
Member

@timhoffm timhoffm Jun 30, 2023

Choose a reason for hiding this comment

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

I'm not quite clear what you mean.

  • We obviously have to continue to support *.mplstyle files
  • We additionally want a (for now internal) .py based style file. We can freely decide on the structure, but __mpl_style__ = {...} seems a reasonable choice - and since it is internal we can still make adjustments.

My concrete suggestion here is to not touch the seaborn .mplstyle files in this PR. It should only introduce stylelib\xkcd.py and the machinery to handle that.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if we want to change seaborn's styles, then instead of this ast.literal_eval work, it'd be simpler to give it an importable name, and do something like:

from . import _seaborn08_common

__mpl_style__ = {
    **_seaborn08_common.__mpl_style__,
    ...,
}

But again, that's only if we want to change these files with inheritance in this manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using imports is tricky because style files currently live in mpl-data and thus not directly importable without playing tricks with the import machinery, which seems to be too complex to be worth it.
If the general agreement is to not change the seaborn styles at all I can just revert that part of the change.

@story645
Copy link
Member

story645 commented Jul 9, 2023

Um, so this came up on the call and is there a motivation for the .py besides the path effects stuff?

The reason why I'm wondering goes back to #24585 and #6738, which talk about writing rcParam files. And reading through, the .py solution would only be for internal style files, at least at first. So if we ever implement a tofile, would it be writing .py files? Or .mplstyle, which means at that point we'd have to figure out a serialization spec for path effects, and what's the barrier now? Basically my concern is this PR is trying to work around serialization and down the line that's gonna create more costs than sorting serialization out would.

I could be missing something major since this is inline w/ @WeatherGod's first proposal, but why wouldn't something really basic like {patheffect function name: {args:, kwargs:}} work?

For the inheritance, the style sheets are implemented as cascading, so could it work to add an 'inherit' parameter and load those in using whatever machinery we currently use for the cascade?

@anntzer
Copy link
Contributor Author

anntzer commented Jul 9, 2023

Um, so this came up on the call and is there a motivation for the .py besides the path effects stuff?

This has been discussed at depth elsewhere in the mplstyle-as-python issues.

The reason why I'm wondering goes back to #24585 and #6738, which talk about writing rcParam files. And reading through, the .py solution would only be for internal style files, at least at first. So if we ever implement a tofile, would it be writing .py files? Or .mplstyle, which means at that point we'd have to figure out a serialization spec for path effects, and what's the barrier now? Basically my concern is this PR is trying to work around serialization and down the line that's gonna create more costs than sorting serialization out would.

.py serialization would actually be much simpler: it would basically just be repr()! (well, except for a few things like patheffects themselves, which need special-casing regardless)

I could be missing something major since this is inline w/ @WeatherGod's first proposal, but why wouldn't something really basic like {patheffect function name: {args:, kwargs:}} work?

That looks not so nice from an actual usability perspective (path.effects: [{"matplotlib.patheffects.withStroke": {"linewidth": 4, "foreground": "w"}}]), plus also a bit of work to implement for (what I think is not a very good reason...)

For the inheritance, the style sheets are implemented as cascading, so could it work to add an 'inherit' parameter and load those in using whatever machinery we currently use for the cascade?

Sure, that's already something that's been requested elsewhere, but I'd leave the implementation to the reader, as again this functionality basically already exists in python (as "import").

@story645
Copy link
Member

story645 commented Jul 10, 2023

That looks not so nice from an actual usability perspective (path.effects: [{"matplotlib.patheffects.withStroke": {"linewidth": 4, "foreground": "w"}}])

I'm thinking more [{'withStroke': : {"linewidth": 4, "foreground": "w"}}] but also that it's more or less equivalent to withStroke(linewidth= in terms of typing (and was also proposed in #6157), and that in terms of usability there's an advanatage to just being able to drop the new patheffects parameters into existing style sheets and keep on w/ the existing abstraction of .mplstyle for aesthetic configurations.

Sure, that's already something that's been requested elsewhere, but I'd leave the implementation to the reader,

Looks like some version started getting implemented in #4240

This has been discussed at depth elsewhere in the mplstyle-as-python issues.

#9528 was what I could find and it doesn't mention any whys and I looked through the issues, meeting notes, and discourse for old mailing lists and am probably using the wrong keyword args since I can't find these discussions. I vaguely remember something about cycler, which I guess is like patheffects and #23012, and are there more use cases or is it always or is it always a general pattern of being a module function?

@timhoffm
Copy link
Member

timhoffm commented Jul 10, 2023

Let’s decouple the discussions: Style inheritance is ortogonal to the style-py topic. It can be solved relatively simple both with or without style-py files. Let’ not discuss this here.

The focus here is to make xkcd usable as a style. This is an API consistency and usability win. I’d be willing to accept internal black magic (non-public py-style files) for that. - Yes, this does not allow other user styles to use path effects. But I’m not aware of any request for this. Maybe xkcd is so special that path effects are not needed anywhere else. (And if it is we can always implement it later on request)

The advantage of internal py-style is that pragmatically solves the xkcd style issue without committing to a new and possibly controversial public style API yet. I’m afraid we won’t solve the general topic soon and if we do not do this special case, xkcd will remain different API for quite some time.

@story645
Copy link
Member

But I’m not aware of any request for this. Maybe xkcd is so special that path effects are not needed anywhere else. (And if it is we can always implement it later on request)

I'm pushing through #26050 b/c I want sketchy 3D #25645 b/c I want a handwritten sketchy style but not full on xkcd. I used to use/adapt https://github.com/petermckeeverPerform/themepy for this<- which pretty sure could be much simpler if path effects was definable in a style file.

My use case is trying to delineate between "this is a figure illustrating a math thing or visualization thing" and "this is a rendered mpl figure" and so I have a whole batch of figures where it'd be nice if I could just define a style. And yes I can just write/hack my own internal, but I've seen enough folks use the sketchy style especially for this kinda theory/applied distinction that I think it could be popular.

I think my thinking is if we can solve imports orthogonally, and patheffects via defining/parsing a spec- I hear @anntzer's concern that the parsing implementation is vaporware and would offer that if I don't put a draft PR for that in in the next month then I'll release my objections- then what advantage do we get to supporting mplstyle as a python file? My meta concern is we're essentially starting down the path of two ways to do the same thing & that really worries me.

@jklymak
Copy link
Member

jklymak commented Jul 11, 2023

I agree that inheritability is a side issue.

I also think simply focusing on an xkcd style is a side issue - as @story645 users may want to write their own style files with paths effects, and I don't think adding an xkcd style is defacto all that useful.

I think the question is do we write a new parser spec for path effects or do we introduce python-based style files? I think if we do python-based style files it would help if there was more justification for why they are useful, and there should be a planned way forward for a public API. If we write a new parser, then someone needs to propose it and implement.

Personally I am leaning towards the flexibility of using python to define styles. It allows substantial flexibility, and the only cost I've heard is it would complicate serialized writing of styles and/or the rcParams.

@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Sep 11, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Sep 12, 2023

would offer that if I don't put a draft PR for that in in the next month then I'll release my objections

@story645 Should we just close #5992 as wontfix (perhaps my preference now, tbh...) or merge this in some form or another?

@WeatherGod
Copy link
Member

WeatherGod commented Sep 12, 2023 via email

@story645
Copy link
Member

@anntzer I think the resolution to #5992 is probably that we agree upon and implement a spec for path rcParams, and I should probably just do that instead of getting sidetracked by docs stuff...

@anntzer
Copy link
Contributor Author

anntzer commented Sep 12, 2023

@WeatherGod As I stated above, my mild preference would in fact now to just close #5992 as wontfix, which is I think the realistic (non-vaporware) outcome of not wanting python-based configuration (something I don't really have the energy to argue for anymore).

@story645
Copy link
Member

I don't want to close #5992 as won't fix b/c it not being a priority doesn't mean it's a thing we refuse to fix.

Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added status: inactive Marked by the “Stale” Github Action status: needs rebase and removed status: inactive Marked by the “Stale” Github Action labels Nov 13, 2023
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.

add xkcd style
7 participants