Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[ENH] xkcd.mplstyle w/ quasi parsing of patheffects.{functions}#26854

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
story645 wants to merge3 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromstory645:path-parse

Conversation

story645
Copy link
Member

@story645story645 commentedSep 21, 2023
edited
Loading

PR summary

Closes#5992 by parsing patheffects, is an alternative to#14943 and also builds on some of the ideas there and in#26050.

@anntzer pointed out that input needs to be a list b/c the same patheffects function can be called multiple times and yield different results, so in this implementationpath.effects can be set to:

  • list of function:[patheffects.Normal(), patheffects.withStroke(linewidth=4)]
  • list of tuples specifying functions:[('Normal', ), ('withStroke', {'linewidth':4})]
  • list of functions + tuples:[patheffects.Normal(), ('withStroke', {'linewidth':4})]
  • string list of tuples('Normal', ), ('withStroke', {'linewidth':4})

I'm gonna defer writing a what's new and all the rest until there's some discussion/consensus on if this is a feasible approach. This PR introduces anxkcd.mplstyle and shims it intoxkcd.plot b/c that was the motivation for this PR, but I can also spin the xkcd stuff out into a separate PR/commit once decisions are more settled.

For the arguments/keywords I went with a very generous "is this an ast_literal parsable dictionary" - advantage is it's a lot cleaner than thepatheffects mini parser I started writing, disadvantage is that it'snot gonna error out til draw time using the function to do the validation.

Definitely not the most optimal/optimized implementation, but this PR sketches out using a function name : dictionary approach, e.g:

path.effects.withStroke: {'linewidth':4,'foreground':'w'}

The advantage of this approach is that we can heavily restrict which patheffects to support and let rc_setup handle that validation, disadvantage is slighlty more typing.

changed some of the path.sketch docs to clarify their format cause misreading them was what caused a lot of the rest of my test failures. spun out#26921 so that may need to go in first

PR checklist

@story645story645 changed the titlexkcd.mplstyle w/ quasi parsing of patheffects.{functions}[ENH] xkcd.mplstyle w/ quasi parsing of patheffects.{functions}Sep 21, 2023
@story645story645 marked this pull request as draftSeptember 21, 2023 16:18
@story645story645 marked this pull request as draftSeptember 21, 2023 16:18
@story645story645 marked this pull request as draftSeptember 21, 2023 16:18
@story645story645 marked this pull request as ready for reviewSeptember 22, 2023 04:55
@story645story645 marked this pull request as draftSeptember 22, 2023 05:10
@story645
Copy link
MemberAuthor

doc build failure mystifies me cause builds locally

@story645
Copy link
MemberAuthor

And it's an important doc build since it's the style example and unicode_minus keyword :/

@story645
Copy link
MemberAuthor

story645 commentedOct 5, 2023
edited
Loading

Note to self:

  • add a figures equal test that plt.xkcd and with style(xkcd) produce the same figure.
  • changename to__name__ orpatheffect or use list[(name, kwarg dict), (name, kwarg dict)]

@tacaswell
Copy link
Member

A concern with using{'name': 'func_name, **func_kwargs} as the structure there is a collision between keys that the API here controls ('name') and the keys that belong to the function that is going to be selected. While it is true none of the functions currently inpatheffects have a keyword named 'name', but it seems unwise to leave that resolvable conflict / special case on the table. Picking another name might reduce the risk of a collision, but usingTuple[str, dict] seems like the safest way to encode this and drops the need for a bunch of text + magic key in the input.

https://www.youtube.com/watch?v=iKzOBWOHGFE has a good section on inband vs out-of-band encoding of information.

@story645
Copy link
MemberAuthor

but using Tuple[str, dict] seems like the safest way to encode this and drops the need for a bunch of text + magic key in the input.

Technically with this format I can do Tuple[str, list, dict] for(name, args, kwargs) but I dunno if that saves anyone work.

@timhoffm
Copy link
Member

Technically with this format I can do Tuple[str, list, dict] for(name, args, kwargs) but I dunno if that saves anyone work.

If you wantTuple[str, list, dict], it should likely beTuple[str, dict] | Tuple[str, list, dict], because I anticipate that the first case is the fare more common one, and empty args'withStroke', (), {'linewidth': 4} is somewhat annoying.

But I suggest to keep it simple and only support kwargs. One could always add the other option later.

story645 reacted with thumbs up emoji

@story645
Copy link
MemberAuthor

It'd be really helpful if#26989 could get in b/c that would help me debug the unicode minus glyph not working error

@story645story645force-pushed thepath-parse branch 2 times, most recently fromfc92806 to45896e8CompareNovember 3, 2023 04:13
@story645
Copy link
MemberAuthor

builds usinghttps://github.com/dummy-index/xkcd-font/raw/brushup/xkcd-script/font/xkcd-script.ttf so do I need to verify that it's okay to use? liscence is cc and using this file b/c it has an extended character set

@story645
Copy link
MemberAuthor

story645 commentedNov 3, 2023
edited
Loading

Also, we may want to ask if we can vendor that file since it's on an unmerged branch of an unmaintained fork if we're gonna use it in doc builds? ETA: or fork that fork or the original project and add that fork?

@story645
Copy link
MemberAuthor

story645 commentedNov 3, 2023
edited
Loading

And do we wanna deprecateplt.xkcd if this goes in? shadowed all the existing xkcd tests so continuity is maintained.

@story645story645force-pushed thepath-parse branch 7 times, most recently from5e9e8a7 to931cf4aCompareNovember 7, 2023 03:32
@tacaswell
Copy link
Member

Can you please put theHumor Sans work in its own PR so we can get that merged and backported ASAP?

@story645story645 mentioned this pull requestNov 9, 2023
5 tasks
@story645
Copy link
MemberAuthor

#27299

@story645
Copy link
MemberAuthor

follow up from call is to make a note in xkcd docs about plt.style as alt

story645and others added3 commitsDecember 1, 2023 14:26
adds tests and stricter validation for path.effects parameterCo-authored-by: Thomas A Caswell <tcaswell@gmail.com>Co-authored-by: Ben Root <ben.v.root@gmail.com>Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
# use patheffects object or instantiate patheffects.object(**kwargs)
return [pe if isinstance(pe, path_effects.AbstractPathEffect)
else getattr(path_effects, pe[0])(**pe[1])
for pe in self._get('path.effects')]
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to support third party patheffects too (e.g.pkgname.modulename.patheffect instead of justwithStroke)

@story645
Copy link
MemberAuthor

notes:
fix doc build - is probably xkcd-font missing symbol
support third party using fully qualified name

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@WeatherGodWeatherGodWeatherGod left review comments

@anntzeranntzeranntzer left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add xkcd style
5 participants
@story645@tacaswell@timhoffm@WeatherGod@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp