-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add xkcd style file. #14943
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
base: main
Are you sure you want to change the base?
Add xkcd style file. #14943
Conversation
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. |
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
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. |
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.
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 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? |
Sorry, I misread you there.
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.
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.
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
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. |
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.
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
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. |
Again, path effects are completely independent of Agg or the (poorly named) Agg filter.
We can make all of them .py files.
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)? |
I got ahead of myself. I am talking about an idea I had years ago to expand
That was sarcasm, right?
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 |
No. They are all builtin styles fully under our control, so there is no security issue with making them python files.
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).
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
I do believe this specific PR handles this specific issue; if you are looking for something else consider opening another issue...
So you agree that distributing mplstyle files (in matplotlibrc syntax) is not best? |
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.
Back around the time
This specific issue is merely a symptom of a deeper problem, and that is that it is currently necessary to set "path.effects" to
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 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. |
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)
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.
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?).
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.
As you mention yourself, both mechanisms already exist, so this is not establishing a new mechanism that's separate from existing mechanisms.
I guess they may work too, though I am curious what advantage you think they may have. |
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.
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? |
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.
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
Already exists, see #6033 (comment) (which can be made into a proper doc entry). I could easily generate a similar template for styles...
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
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.
Do you mean specifically the ability to enumerate styles? |
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. |
Rebased, per #5992 (comment). |
|
||
|
||
__mpl_style__ = { | ||
**ast.literal_eval(Path(__file__).with_name("_seaborn-v0_8-common.py").read_text()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 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? |
This has been discussed at depth elsewhere in the mplstyle-as-python issues.
.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)
That looks not so nice from an actual usability perspective (
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"). |
I'm thinking more
Looks like some version started getting implemented in #4240
#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? |
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. |
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. |
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. |
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. |
I would like to reiterate my long-standing aversion to python-based
configuration files, since it is arbitrary code execution of code that is
not installed via trusted means.
…On Tue, Sep 12, 2023 at 10:58 AM Antony Lee ***@***.***> wrote:
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 <https://github.com/story645> Should we just close #5992
<#5992> as wontfix
(perhaps my preference now, tbh...) or merge this in some form or another?
—
Reply to this email directly, view it on GitHub
<#14943 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6DGWVEIG73Y44DCBBLX2B2C7ANCNFSM4IIHVHRQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@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). |
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. |
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. |
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