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

Improvements to alt.themes #3519

Open
dangotbanned opened this issue Aug 4, 2024 · 18 comments · Fixed by #3526
Open

Improvements to alt.themes #3519

dangotbanned opened this issue Aug 4, 2024 · 18 comments · Fixed by #3526

Comments

@dangotbanned
Copy link
Member

dangotbanned commented Aug 4, 2024

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 as (VegaThemes: Literal[...])
  • Define altair themes as (AltairThemes: Literal["opaque", ...)
  • _ThemeName -> DefaultThemes: TypeAlias = AltairThemes | VegaThemes
  • Annotate themes.enable(name: DefaultThemes)
  • Revisit the need for defining this across two theme.py modules, as part of Flatten the package structure #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 pola-rs/polars#17995 (comment), I thought we could improve both the docs and the UX itself.


Ideas

Feat

Docs

Refactor

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

@mattijn
Copy link
Contributor

mattijn commented Aug 4, 2024

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
Copy link
Member Author

dangotbanned commented Aug 4, 2024

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
Copy link
Contributor

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 #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
Copy link
Contributor

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.

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
Copy link
Member Author

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

SCHEMA_URL_TEMPLATE: Final = "https://vega.github.io/schema/{library}/{version}.json"


@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
Copy link
Contributor

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
Copy link
Member Author

dangotbanned commented Aug 5, 2024

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 vega/vl-convert#39 and feel dumb for not realising vl_convert already can return the full theme definitions.
Since @mattijn's #3519 (comment) I thought vlc.get_themes() was only returning the names 🤦‍♂️

Screenshot

image

Please disregard all of #3519 (comment)

dangotbanned added a commit to dangotbanned/altair that referenced this issue Aug 5, 2024
dangotbanned added a commit to dangotbanned/altair that referenced this issue Aug 7, 2024
dangotbanned added a commit to dangotbanned/altair that referenced this issue Aug 7, 2024
Resolves the *long-term* solution mentioned in vega#3519 (comment)
dangotbanned added a commit to dangotbanned/altair that referenced this issue Aug 8, 2024
binste pushed a commit that referenced this issue Aug 8, 2024
binste pushed a commit that referenced this issue Aug 8, 2024
* feat: Adds `vega-themes.json` using `vl_convert`

#3519 (comment)

* Update vega-themes.json

* fix: Force LF

* fix: Use `sort_keys` for deterministic `vega-themes.json`

* build: run `generate-schema-wrapper`

* ci: Introduce `vl_convert` dependency to GH `Test that schema generation has no effect`

#3519 (comment)

https://github.com/vega/altair/actions/runs/10263632193/job/28395892206?pr=3523

* ci: Output diff when schema generation has an effect

Not sure of the right command here, as I can't repro locally

* ci: Change gitattributes to always LF

* fix: re-run with fresh env

#3523 (comment)

* feat(typing): Generate `VegaThemes` alias

Resolves the *long-term* solution mentioned in #3519 (comment)
@dangotbanned
Copy link
Member Author

dangotbanned commented Aug 10, 2024

  • Creating a TypedDict for the theme config object

@binste I briefly mentioned in #3523 (comment) 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

def generate_vegalite_config_mixin(schemafile: Path) -> tuple[list[str], str]:
imports = [
"from . import core",
"from altair.utils import use_signature",
]
class_name = "ConfigMethodMixin"
code = [
f"class {class_name}:",
' """A mixin class that defines config methods"""',
]
with schemafile.open(encoding="utf8") as f:
schema = json.load(f)
info = SchemaInfo({"$ref": "#/definitions/Config"}, rootschema=schema)
# configure() method
method = CONFIG_METHOD.format(classname="Config", method="configure")
code.append("\n ".join(method.splitlines()))
# configure_prop() methods
for prop, prop_info in info.properties.items():
classname = prop_info.refname
if classname and classname.endswith("Config"):
method = CONFIG_PROP_METHOD.format(classname=classname, prop=prop)
code.append("\n ".join(method.splitlines()))
return imports, "\n".join(code)

generate_vegalite_mark_mixin

Relevant since this deals with typing, whereas the config equivalent does not.

def generate_vegalite_mark_mixin(
schemafile: Path, markdefs: dict[str, str]
) -> tuple[list[str], str]:
with schemafile.open(encoding="utf8") as f:
schema = json.load(f)
class_name = "MarkMethodMixin"
imports = [
"from typing import Any, Sequence, List, Literal, Union",
"",
"from altair.utils.schemapi import Undefined, UndefinedType",
"from . import core",
]
code = [
f"class {class_name}:",
' """A mixin class that defines mark methods"""',
]
for mark_enum, mark_def in markdefs.items():
if "enum" in schema["definitions"][mark_enum]:
marks = schema["definitions"][mark_enum]["enum"]
else:
marks = [schema["definitions"][mark_enum]["const"]]
info = SchemaInfo({"$ref": f"#/definitions/{mark_def}"}, rootschema=schema)
# adapted from SchemaInfo.init_code
arg_info = codegen.get_args(info)
arg_info.required -= {"type"}
arg_info.kwds -= {"type"}
def_args = ["self"] + [
f"{p}: "
+ info.properties[p].get_python_type_representation(
for_type_hints=True,
additional_type_hints=["UndefinedType"],
)
+ " = Undefined"
for p in (sorted(arg_info.required) + sorted(arg_info.kwds))
]
dict_args = [
f"{p}={p}" for p in (sorted(arg_info.required) + sorted(arg_info.kwds))
]
if arg_info.additional or arg_info.invalid_kwds:
def_args.append("**kwds")
dict_args.append("**kwds")
for mark in marks:
# TODO: only include args relevant to given type?
mark_method = MARK_METHOD.format(
mark=mark,
mark_def=mark_def,
def_arglist=", ".join(def_args),
dict_arglist=", ".join(dict_args),
)
code.append("\n ".join(mark_method.splitlines()))
return imports, "\n".join(code)

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:

  • There is only 1 class listed
  • len(SchemaInfo.title) <= some_limit

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
Copy link
Contributor

binste commented Aug 15, 2024

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?

  • A ThemeConfig typed dict, as you are now developing it in feat: Adds ThemeConfig (TypedDict) #3536, gives all the flexibility but can be overwhelming for new joiners.
  • 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:
  • font size (or just scale it) (Add simpler ways of controlling element and font size #2820)
  • rotate y axis titles so that they are horizontal on top of the axis
  • remove border around chart (view.strokeWidth: 0)
  • ...

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
Copy link
Member Author

@binste I will try to go through #3519 (comment) 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
Copy link
Member Author

dangotbanned commented Aug 16, 2024

dangotbanned added a commit that referenced this issue Sep 5, 2024
* feat: Adds `@register_theme` decorator

Resolves one item in #3519

* build: run `update-init-file`

Adds `@register_theme` to top-level

* test: Adds `test_register_theme_decorator`

* refactor(typing): Specify `dict[str, Any]` instead of `dict[Any, Any]`

The latter may give false-positives for json-incompatible dicts

---------

Co-authored-by: Stefan Binder <[email protected]>
@dangotbanned dangotbanned reopened this Sep 5, 2024
@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 9, 2024

Moved to #3586

Make it easier for downstream libraries 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 would still be possible

Using a theme would have the benefit of deferring these config settings until the Chart is rendered - placing them in the top-level only.


It might be worth seeing if we can come to a good solution to this as part of #3519 since we have already discussed issues 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

themes.register(
"default",
lambda: {"config": {"view": {"continuousWidth": 300, "continuousHeight": 300}}},
)
themes.register(
"opaque",
lambda: {
"config": {
"background": "white",
"view": {"continuousWidth": 300, "continuousHeight": 300},
}
},
)
themes.register("none", dict)
for theme in VEGA_THEMES:
themes.register(theme, VegaTheme(theme))
themes.enable("default")

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.

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

@dangotbanned
Copy link
Member Author

@joelostblom what do you think about doing something like #2593 but with a reduced scope to recreate Vega Theme Test?

Docs

@joelostblom
Copy link
Contributor

I think that could be interesting, do you mean with a full interactive cell so that readers could interact with individual options in the theme? Rather than just a dropdown to select between existing themes like in the Vega Themes pages? I think this is already possible via replite .

Long term, I think it would be neat if all code cells would be optionally interactive. Something like what the Panel docs have where you can click a button to execute the content in the cell (but in our case it should also be editable).

Reflecting on @binste 's config above, I would be in favor for increasing the font sizes etc of the default altair theme, given that we are already having the charts being 300x300 instead of the vega default of 200x200 and it would make sense if fonts etc were a bit bigger then too. Maybe a v6 feature.

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 18, 2024

I think that could be interesting, do you mean with a full interactive cell so that readers could interact with individual options in the theme? Rather than just a dropdown to select between existing themes like in the Vega Themes pages? I think this is already possible via replite .

@joelostblom I haven't dived into the various jupyter... tools, so I'm open to anything you think might be useful here.
From my (limited) understanding, a 1:1 copy should be possible.
However, if you see a path to interacting with individual options - that sounds even better!

Vega Theme Test

  • Contains only 8 charts
  • Most (maybe all?) have an altair equivalent in example-gallery

To me these seem like some nice constraints to work with, and would definitely be easier to get merged than #2593.
Anything learned in the process could also help with long term goals you have for a more interactive User Guide

Edit

See clarification in #3519 (comment)

@joelostblom
Copy link
Contributor

From my (limited) understanding, a 1:1 copy should be possible.

If we were to do a one to one copy, wouldn't it be easier to contribute the new themes upstream so that they are included in the already existing Vega page? Maybe that is the easiest implementation to get started, and a later follow up could be to create the interactive cells I mentioned (I would also be ok to go the interactive way from the start, but I am personally a bit low on time right now to explore if what I suggested is the most suitable option here)

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 18, 2024

From my (limited) understanding, a 1:1 copy should be possible.

If we were to do a one to one copy, wouldn't it be easier to contribute the new themes upstream so that they are included in the already existing Vega page? Maybe that is the easiest implementation to get started, and a later follow up could be to create the interactive cells I mentioned (I would also be ok to go the interactive way from the start, but I am personally a bit low on time right now to explore if what I suggested is the most suitable option here)

Apologies @joelostblom , what I wrote and what I was thinking about didn't really line up 😅

By 1:1 I meant creating the charts and arranging them using altair code, which would serve two purposes:

  1. Simply mirroring the theme previewing behavior from the original, using the same default themes
  2. Providing the sample code anyone authoring a new theme could use when experimenting

Personally, this would've saved me a lot of time trying to debug edge cases where something that worked well for one spec - didn't translate as well for another.

We could always try that approach if you'd see any value in it - but I'm still happy to wait for a more interactive experience

@joelostblom
Copy link
Contributor

Ah I see, yes I think that could be helpful, especially the second part about people creating themes being able to quickly try them out with multiple charts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants