search

vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.23k stars 784 forks source link

Improvements to `alt.themes` #3519

Open dangotbanned opened 1 month ago

dangotbanned commented 1 month ago

What is your suggestion?

Following discussion w/ @binste in

Original comment relating to autocomplete For a *long-term* solution, I'd want to: - Extract the names directly from [source](https://github.com/vega/vega-themes/blob/main/src/index.ts) as (`VegaThemes: Literal[...]`) - Define `altair` themes as (`AltairThemes: Literal["opaque", ...`) - [Your comment](https://github.com/pola-rs/polars/pull/17995#issuecomment-2266683585) which was what sparked this PR 😄 - [changing-the-theme](https://altair-viz.github.io/user_guide/customization.html#changing-the-theme) which states a plan to add more in the future - ~~`_ThemeName`~~ -> `DefaultThemes: TypeAlias = AltairThemes | VegaThemes` - Annotate `themes.enable(name: DefaultThemes)` - Revisit the need for defining this across two `theme.py` modules, as part of https://github.com/vega/altair/issues/3337

The larger goal would be making the process of creating/choosing a theme as easy as possible.

After linking to the existing docs in https://github.com/pola-rs/polars/pull/17995#issuecomment-2266657656, I thought we could improve both the docs and the UX itself.

Ideas

Feat

Docs

I know that @binste had some ideas as well, maybe others do too

mattijn commented 1 month ago

Maybe useful, maybe not, but themes are also known within vl-convert:

import vl_convert as vlc
list(vlc.get_themes().keys())
['carbong10',
 'carbong100',
 'carbong90',
 'carbonwhite',
 'dark',
 'excel',
 'fivethirtyeight',
 'ggplot2',
 'googlecharts',
 'latimes',
 'powerbi',
 'quartz',
 'urbaninstitute',
 'vox']
dangotbanned commented 1 month ago

Maybe useful, maybe not, but themes are also known within vl-convert:

import vl_convert as vlc
list(vlc.get_themes().keys())

Thanks @mattijn, that seems to end up at https://github.com/vega/vl-convert/blob/89bbda91334494fbcf29144b4c1fe7b6ed18168e/vl-convert-python/src/lib.rs#L1028-L1044

If I've understood this correctly (maybe @jonmmease can correct), this seems like the equivalent of asking vega-lite what is on the menu?

This would be a pretty simple solve for keeping the Literal updated. But would mean depending on vl_convert during tools/generate_schema_wrapper.py, to check if we need to update

joelostblom commented 1 month ago

Related to improving the experience of modifying chart styles is making it easier to modify common elements in existing themes, e.g. setting a larger default font size as tracked in this issue https://github.com/vega/altair/issues/2820 and the various comments linked there. It's a bit outside the scope of what you are suggesting here @dangotbanned but I wanted to link it so that you were aware of some of the previous discussion we've had on similar topics.

jonmmease commented 1 month ago

If I've understood this correctly (maybe @jonmmease can correct), this seems like the equivalent of asking vega-lite what is on the menu?

Hi @dangotbanned, yes vl-convert vendors the vega-themes JavaScript library (which vega-lite pulls themes from), and extracts the theme definitions from there. I'm in the process of updating it to use the new vega-themes release that includes the carbon themes in https://github.com/vega/vl-convert/pull/178.

I think depending on vl-convert during generate_schema_wrapper.py is reasonable, It's already a requirement for building the docs and running the image export tests.

My motivation for adding this info to vl-convert was so that we could eventually let Altair users inspect the definitions of these themes to use them as starting points for their own themes. I haven't thought it through in detail, but I wonder if we should inline the full theme definitions during generate_schema_wrapper.py so we have them in Altair.

dangotbanned commented 1 month ago

If I've understood this correctly (maybe @jonmmease can correct), this seems like the equivalent of asking vega-lite what is on the menu?

Hi @dangotbanned, yes vl-convert vendors the vega-themes JavaScript library (which vega-lite pulls themes from), and extracts the theme definitions from there. I'm in the process of updating it to use the new vega-themes release that includes the carbon themes in vega/vl-convert#178.

Thanks for the explanation @jonmmease! That wasn't what I thought was happening, glad you cleared it up

My motivation for adding this info to vl-convert was so that we could eventually let Altair users inspect the definitions of these themes to use them as starting points for their own themes. I haven't thought it through in detail, but I wonder if we should inline the full theme definitions during generate_schema_wrapper.py so we have them in Altair.

I definitely agree with the end goal, but had an alternative way to get there.

Would it be in scope for https://github.com/vega/vega-themes/ to output (during CI) a .json of all current themes? I'm thinking it would be quite easy to have one file, mapping the theme names to each config. Especially if we only consider the final export theme-powerbi.ts#L113 - rather than all the parameterization in the full .ts.

I think depending on vl-convert during generate_schema_wrapper.py is reasonable, It's already a requirement for building the docs and running the image export tests.

Looking at the frequency of vega-themes releases this might be a non-issue, and if bumping all of these is trivial. However, my hesitation would be that were a new theme added, an altair user needs the latest altair which requires vl-convert to pull the new themes and make a new release, w/ altair releasing on that new version.

Simply reading the latest .json from vega-themes would ensure each release of altair is always up-to-date. Also wouldn't look out of place with since we already grab the schema from the github https://github.com/vega/altair/blob/95f654fcd036069e1101883d176ec67c6f13fa25/tools/generate_schema_wrapper.py#L39

@jonmmease do you think this would be a reasonable ask? If not then I think using vl-convert would be the way to go 👍

jonmmease commented 1 month ago

Are you picturing that this .json file would be loaded only when generate_schema_wrapper.py runs, or also at runtime? I would lean against loading it at runtime since this introduces a runtime dependency on an internet connection, which we don't want to introduce if we can help it. vl-convert vendors everything it needs so that operations like image export work offline.

But if it's only at code generation time, then that seems perfectly reasonable, though I don't know if there are a lot of benefits over using vl-convert in that case.

dangotbanned commented 1 month ago

Are you picturing that this .json file would be loaded only when generate_schema_wrapper.py runs, or also at runtime?

Apologies for being unclear, yeah I meant updated at on each generate_schema_wrapper.py run. Absolutely agree on avoiding needing an internet connection.

But if it's only at code generation time, then that seems perfectly reasonable, though I don't know if there are a lot of benefits over using vl-convert in that case.

That's fair, you certainly know more than I do about the ease of coordinating between the two projects.

I wonder if we should inline the full theme definitions during generate_schema_wrapper.py so we have them in Altair

If you can figure this out with vl-convert we should go with that @jonmmease

Edit

Just finished reading through https://github.com/vega/vl-convert/pull/39 and feel dumb for not realising vl_convert already can return the full theme definitions. Since @mattijn's https://github.com/vega/altair/issues/3519#issuecomment-2267624907 I thought vlc.get_themes() was only returning the names 🤦‍♂️

image

Please disregard all of https://github.com/vega/altair/issues/3519#issuecomment-2269004203

dangotbanned commented 1 month ago
  • Creating a TypedDict for the theme config object

@binste I briefly mentioned in https://github.com/vega/altair/pull/3523#issuecomment-2273140252 that this might take a while for me to do.

Thought I'd add some notes here, prior to a PR, in case you (or anyone else) were more familiar with codegen.py

Notes

Prior Art

So far, these seem like the relevant starting points/outputs:

PR

Code

core.Config GitHub doesn't want to show the preview, but the link will still highlight inline. https://github.com/vega/altair/blob/f0c1e0a77bf8e3660fdabf484229a47a021d1f8c/altair/vegalite/v5/schema/core.py#L5053-L5407
generate_vegalite_config_mixin In terms of selecting properties that may need a `TypedDict` https://github.com/vega/altair/blob/f0c1e0a77bf8e3660fdabf484229a47a021d1f8c/tools/generate_schema_wrapper.py#L766-L792
generate_vegalite_mark_mixin Relevant since this deals with typing, whereas the *config* equivalent does not. https://github.com/vega/altair/blob/f0c1e0a77bf8e3660fdabf484229a47a021d1f8c/tools/generate_schema_wrapper.py#L704-L763

Related

While reading through these, core.Config seemed like an example where we could be more specific in the type annotations - rather than SchemaBase.

Not sure how common the scenario would be, but writing some logic to use the actual type when:

For cases like the above, it would mean a user wouldn't need to refer to the docs as often - if their IDE/etc can utilise the annotations

binste commented 1 month ago

It's exciting to see this 🚀 I'll try to contribute through comments soon. For now, just some drive-by thoughts in case you find them useful:

How could the bigger picture UX look like for themes?

Writing this out, we should have a solution which allows for the full flexibility (dict/typed dict) and I do see the appeal of a light convenience function/class to get started with a new theme.

Inspiration

dangotbanned commented 1 month ago

@binste I will try to go through https://github.com/vega/altair/issues/3519#issuecomment-2292010192 in detail tomorrow, but wanted to say really appreciate all the thought you've put in to the UX!

Just finished the most significant part of #3536 with bb99389 (#3536). Still a way off from review, but the end-result is ready for playing around with 😎

Maybe we want a "theme builder" class instead/a function which produces the ThemeConfig. Or is this hiding complexity which should not be hidden? It could have arguments/methods to set some commonly used characteristics such as ...

Sounds interesting! I'll say I fully didn't anticipate the level of customization you can do with a theme. Having the option to ease into/simplify seems worth exploring IMO

dangotbanned commented 1 month ago

@binste I think the key missing component of theme documentation is demonstrating patterns like in https://github.com/dangotbanned/altair/blob/c35324d5cd15bf04fbc47efa611837eefd20e138/altair/vegalite/v5/theme.py#L77-L133

This wouldn't require any additional functions/classes and would be "teaching" theme generation in a way that closely mirrored vega-themes:

Looking at seaborn.set_theme, this is pretty much the same idea but with 1 of 4 scale constants applied in sns.rcmod.plotting_context

dangotbanned commented 6 days ago
Moved to #3586 ## Make it easier for [downstream libraries](https://github.com/pola-rs/polars/pull/18609#discussion_r1749814694) to *safely* contribute themes
Originally posted by @dangotbanned in discussion w/ @MarcoGorelli

As I understand, the `alt.Chart.configure_` calls are being used to avoid registering + enabling a theme - which could override a user's custom theme. These work fine in isolation, but AFAIK would have issues if a user were to layer/concat/facet the result - since `config` is only valid at the top-level. You might want to add tests to see if [these ops](https://github.com/vega/altair/blob/df14929075b45233126f4cfe579c139e0b7f0559/tests/vegalite/v5/test_api.py#L332-L361) would still be possible - https://altair-viz.github.io/user_guide/customization.html#global-config - https://altair-viz.github.io/user_guide/configuration.html#top-level-chart-configuration Using a theme would have the benefit of deferring these config settings until the `Chart` is rendered - placing them in the [top-level only](https://github.com/vega/altair/blob/df14929075b45233126f4cfe579c139e0b7f0559/altair/vegalite/v5/api.py#L1837). --- It might be worth seeing if we can come to a good solution to this as part of https://github.com/vega/altair/issues/3519 since we have already [discussed](https://github.com/pola-rs/polars/pull/17995#issuecomment-2282795126) [issues](https://github.com/pola-rs/polars/pull/17995#issuecomment-2282802508) with the theme route

### Problem A library like `polars` may wish to provide a default theme, but not override a user-defined or user-enabled theme. AFAIK, the "best" solution for this right now would be to override our `"default"` theme. However, this would be a **destructive** action and wouldn't scale well to multiple 3rd-parties each doing so:
Code block https://github.com/vega/altair/blob/df14929075b45233126f4cfe579c139e0b7f0559/altair/vegalite/v5/theme.py#L56-L74
### Solution(s) We could extend `ThemeRegistry` to support priority levels. Either when registering/enabling a theme a level will be set corresponding to the party. ```py from enum import IntEnum class ThemePriority(IntEnum): USER = 1 THIRD_PARTY = 2 DEFAULT = 3 # alternatives: `ALTAIR`, `STANDARD`, `BUILTIN` ``` For backwards-compatibility, this **must** default to `ThemePriority.USER` in any signatures the argument can be passed in from. All themes defined/registered in https://github.com/vega/altair/blob/df14929075b45233126f4cfe579c139e0b7f0559/altair/vegalite/v5/theme.py will be assigned `ThemePriority.DEFAULT`. The semantics of which theme should be enabled for `ThemePriority.(USER|DEFAULT)` are quite simple. The highest priority (lowest-valued) **enabled** theme is selected: - User *does not* enable a theme, no changes from existing behavior - User *enables* a theme with `ThemePriority.DEFAULT`, no changes from existing behavior - User *enables* a theme with `ThemePriority.USER`, no changes from existing behavior - User *disables* a theme with `ThemePriority.USER`, falls back to the last enabled `ThemePriority.DEFAULT` The basic resolution implementation for `ThemePriority.THIRD_PARTY` would be identical to the above. Simply a way for 3rd-parties to **opt-in** for a way to safely be used instead of the defaults - but not over user themes. However, I think this behavior *itself* should be pluggable - to support alternative resolution semantics like: - Third-party wants to enable their theme for the lifetime of `ChartType`(s) they produce? - Either strictly or as a preference only - User wants to **opt-out** of third-party contributions? - Multiple third-parties? #### Related - https://docs.python.org/3/library/queue.html#queue.PriorityQueue - https://docs.python.org/3/library/enum.html - https://github.com/pytest-dev/pluggy