search

vega / altair

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

feat: Adds `ThemeConfig` (`TypedDict`) #3536

Closed dangotbanned closed 2 months ago

dangotbanned commented 3 months ago

Resolves one item in #3519

Description

This PR aims to improve the UX when authoring a theme. By utilising TypedDict(s) we can provide a very similar experience to that of writing in Vega Editor, with rich autocompletion and static validation feedback.

Main Additions

Interactions with public API

These classes are entirely isolated from the main alt.___ namespace, but can still be used anywhere a dict|Map annotation is present in existing code. Currently:

Using the classes themselves is entirely optional, as can be seen in test_theme.py. Simply annotating ThemeConfig (and using a static type checker) allows the user to specify a theme using native python types.

Examples

Autocomplete w/ native python types

Code block

```py import altair as alt from altair.typing import ThemeConfig # noqa: TCH001 @alt.register_theme("custom theme", enable=True) def my_theme() -> ThemeConfig: return {""} ```

image

Type checking w/ mixed python|altair types

Code block

```py import altair as alt from altair.typing import ThemeConfig # noqa: TCH001 from altair.typing.theme import ConfigKwds @alt.register_theme("custom theme", enable=True) def my_theme() -> ThemeConfig: return { "autosize": {"contains": "padding"}, "config": ConfigKwds(font="Segoe UI", axis=9999999), "width": "container", "height": [500], } ```

image

Options for imports & runtime typing

Code block

```py import altair as alt from altair.typing import ThemeConfig, theme # noqa: F811 from altair.typing.theme import AxisConfigKwds from vega_datasets import data custom_theme = ThemeConfig( config=theme.ConfigKwds( axis=AxisConfigKwds(grid=True, labelFont="system-ui"), axisX=AxisConfigKwds(labelOpacity=0.5), circle=theme.MarkConfigKwds(fill="purple"), ) ) print( f"{custom_theme!r}\n\n" f"The above theme was defined using {ThemeConfig.__name__!r}, " f"but at runtime the type is {type(custom_theme).__name__!r}" ) # Manual registration alt.themes.register("demo", lambda: custom_theme) alt.themes.enable("demo") alt.Chart(data.cars()).mark_circle().encode(x="Horsepower", y="Miles_per_Gallon") ```

image

@register_theme doc

Default

Code block

```py import altair as alt from altair.typing import ThemeConfig from altair.typing.theme import ConfigKwds from vega_datasets import data source = data.stocks() lines = ( alt.Chart(source, title=alt.Title("Stocks")) .mark_line() .encode(x="date:T", y="price:Q", color="symbol:N") .interactive(bind_y=False) ) lines ```

image

With a registered theme

Code block

```py @alt.register_theme("param_font_size", enable=True) def custom_theme_2() -> ThemeConfig: sizes = 12, 14, 16, 18, 20 return { "autosize": {"contains": "content", "resize": True}, "background": "#F3F2F1", "config": ConfigKwds( axisX={"labelFontSize": sizes[1], "titleFontSize": sizes[1]}, axisY={"labelFontSize": sizes[1], "titleFontSize": sizes[1]}, font="'Lato', 'Segoe UI', Tahoma, Verdana, sans-serif", headerColumn={"labelFontSize": sizes[1]}, headerFacet={"labelFontSize": sizes[1]}, headerRow={"labelFontSize": sizes[1]}, legend={"labelFontSize": sizes[0], "titleFontSize": sizes[1]}, text={"fontSize": sizes[0]}, title={"fontSize": sizes[-1]}, ), "height": {"step": 28}, "width": 350, } lines ```

image

Early POC Demo

These screenshots were taken mid-development

Providing fully nested auto-complete ![image](https://github.com/user-attachments/assets/1ab8e3c1-2dc9-4aa3-aada-f402f556f3f2)
Type checking feedback

![image](https://github.com/user-attachments/assets/84b70a50-8cc0-4246-9e72-ad472a12d77f)

Tasks

dangotbanned commented 3 months ago

@binste FYI I spent a good chunk of yesterday trying (and failing) to make progress on this point. If you have any ideas I'm all ears - will loop back to this eventually but it really stumped me

Tasks

  • [ ] Replace MANUAL_DEFS w/ a recursive/graph-based solution
    • Need a better understanding of the existing code
    • Working through changes like 3c9aab5 (#3536)
dangotbanned commented 2 months ago

Think I need to account for ThemeConfig being the type of a key "config" in some outer schema.

Maybe adapting TopLevelUnitSpec?

Update 1

Planning on using the following subset of properties, which without any extra code would be:

class StillUnnamedTopLevel(TypedDict):
    align: RowColLayoutAlign | LayoutAlign_T
    autosize: AutoSizeParamsKwds | AutosizeType_T
    background: ColorHex | ColorName_T
    bounds: Literal['full', 'flush']
    center: bool | RowColboolean
    config: Config # <------------------------- `ThemeConfig`
    description: str
    height: Step | float | Literal['container']
    name: str
    padding: Map | float
    params: Sequence[VariableParameterKwds | TopLevelSelectionParameterKwds]
    projection: Projection
    resolve: Resolve
    spacing: float | RowColnumber
    title: str | TitleParams | Sequence[str]
    usermeta: Map
    view: ViewBackground
    width: Step | float | Literal['container']

Tasks

@binste pinging in case you had any thoughts here?

The first thing I tried was simply passing TopLevelUnitSpec to find_theme_config_targets.

However, that had a dramatic effect on the number of TypedDict(s):

Personally, I've only used these keys in a theme before - with almost all of the contents being "config":

{"autosize", "background", "config", "height", "width"}

So I'm trying to avoid creating loads of things that aren't used frequently

Update 2

dangotbanned commented 2 months ago

This PR blows my mind! 🤯 Really appreciate all the work you put into it, this obviously required going very deep in the VL schema and the code generation and I think the outcome is super helpful for further advancing the UX of themes. Now I really want to write some new themes 😄

I only have some very minor comments. If you feel good with it, I'm ok if you merge it afterwards!

Thanks so much for the review @binste! (and helping me get unstuck midway through)

Hopefully this will make it easier to explore some of your points in https://github.com/vega/altair/issues/3519#issuecomment-2292010192 once we've merged

I'll leave this open for a few hours in case you had any thoughts on https://github.com/vega/altair/pull/3536#discussion_r1761463506 - otherwise I'll follow this up with another PR as mentioned

mattijn commented 2 months ago

Thanks for all the work. A concise PR description with an explanation of the what/why/how would really help future readers coming to this PR. This description is also helpful for making the release notes better. Thanks again!

dangotbanned commented 2 months ago

Thanks for all the work. A concise PR description with an explanation of the what/why/how would really help future readers coming to this PR. This description is also helpful for making the release notes better. Thanks again!

No worries, will work on that now thanks @mattijn

dangotbanned commented 2 months ago

@mattijn just updated the description, hopefully you (and others) find it helpful 😄

mattijn commented 2 months ago

Thanks! Can you add also how this feature is being used? Do we want users to import the following? 

import altair.vegalite.v5.schema._config as theme

And

from altair.vegalite.v5.schema._config import ThemeConfig

And then? I'd love to have a brief description that explains how a new user can start using this feature. It would be great if you could provide a simple example that highlights just one feature controlled by the ThemeConfig. Thanks again for all your hard work!

joelostblom commented 2 months ago

Thanks for expanding on the PR description, that was helpful for me to appreciate just how much this PR improves the experience of defining custom themes, wow! In line with what Mattijn suggested, it would be great to update the custom theme section of the docs to use this approach and mention the autocompletion behavior.

dangotbanned commented 2 months ago

Thanks! Can you add also how this feature is being used? Do we want users to import the following?

import altair.vegalite.v5.schema._config as theme

Correction (1)

from altair.typing import theme

Usage

from altair.typing import ThemeConfig, theme

custom_theme = ThemeConfig(
    config=theme.ConfigKwds(
        axis=theme.AxisConfigKwds(grid=True, labelFont="system-ui"),
        circle=theme.MarkConfigKwds(fill="purple"),
    )
)
custom_theme

And

from altair.vegalite.v5.schema._config import ThemeConfig

Correction (2)

from altair.typing import ThemeConfig

And then? I'd love to have a brief description that explains how a new user can start using this feature. It would be great if you could provide a simple example that highlights just one feature controlled by the ThemeConfig. Thanks again for all your hard work!

Reading this made me spot a bug with the import behavior, just fixed in d4bd6db (#3536)

For reference, @register_theme has an example of the intended import path

https://github.com/dangotbanned/altair/blob/6e4af95becc8ff9be033535d45ce3db9b2c749a3/altair/vegalite/v5/theme.py#L94-L135

dangotbanned commented 2 months ago

@joelostblom @mattijn thanks for your comments!

I'll be updating the description with more examples tomorrow, but hopefully you get a rough idea with d4bd6db (#3536)

it would be great to update the custom theme section of the docs to use this approach and mention the autocompletion behavior.

Absolutely agree, I added that to the list in https://github.com/vega/altair/issues/3519#issue-2447214476 an hour ago - I think it will make the remaining items much easier

mattijn commented 2 months ago

Awesome! Looking forward to! Another question I have, why is this ThemeConfig not imported to the root level of Altair? It sounds like a top level object to me. Can you elaborate on this? With feature comes questions😀. Thanks!

dangotbanned commented 2 months ago

Awesome! Looking forward to! Another question I have, why is this ThemeConfig not imported to the root level of Altair? It sounds like a top level object to me. Can you elaborate on this? With feature comes questions😀. Thanks!

For v6 I would agree, but right now I think this is much easier to find. (https://github.com/vega/altair/issues/2918)

I can't seem to find the thread, but I suggested to @binste having a small number of ...Kwds available in altair.typing - with the rest in altair.typing.theme.

With the current layout, I think this reinforces the relationship between ThemeConfig -> ...Kwds and that ThemeConfig is the entry point.

I'm not sure if all that made sense, if not apologies, I was supposed to call it a day already 😅

dangotbanned commented 2 months ago

@mattijn @joelostblom

FYI the conversation marked as unresolved is actually resolved.

I haven't updated the status of it yet, to block merging until you've both seen the updated description

Otherwise this should be ready to merge 👍

joelostblom commented 2 months ago

I don't have much to comment on the changes to the import, so will refer to @mattijn for that.

On first look, the class based syntax looks maybe a bit verbose:

image

So I would probably opt for the combination with dicts myself:

image

But maybe I will change my mind when I try them; it's great that both exist and support autocompletion!

dangotbanned commented 2 months ago

@mattijn I'm planning to merge this tomorrow if you don't have any objections.

In #3600 I've been trying to work around not having the refactored tools/... and have made a lot of progress; but I need to start making changes in tools.generate_schema_wrapper.py and tools.schemapi.utils.py soon for generating alt.expr.__init__.py

mattijn commented 2 months ago

Thanks for asking! I still cannot understand why this ThemeConfig feature is available within the altair.types. I thought we declared that altair.types is the interface of public types within altair. But now we are placing features such as ThemeConfig in there. Is it a type? Therefore it does not help me with discoverability either.

Also, as a user, I really prefer a feature explanation that I can copy and paste. I don't see that in this PR. I see a few screenshots, but that is really not the advocated route for a Minimal, Reproducible Example. For me it is still not clear how I can use this from the PR description.

I found the following in the code:

import altair as alt
from altair.typing import ThemeConfig
from vega_datasets import data

@alt.register_theme("param_font_size", enable=True)
def custom_theme() -> ThemeConfig:
    sizes = 12, 14, 16, 18, 20
    return {
        "autosize": {"contains": "content", "resize": True},
        "background": "#F3F2F1",
        "config": {
            "axisX": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            "axisY": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            "font": "'Lato', 'Segoe UI', Tahoma, Verdana, sans-serif",
            "headerColumn": {"labelFontSize": sizes[1]},
            "headerFacet": {"labelFontSize": sizes[1]},
            "headerRow": {"labelFontSize": sizes[1]},
            "legend": {"labelFontSize": sizes[0], "titleFontSize": sizes[1]},
            "text": {"fontSize": sizes[0]},
            "title": {"fontSize": sizes[-1]},
        },
        "height": {"step": 28},
        "width": 350,
    }

source = data.stocks()
lines = (
    alt.Chart(source, title=alt.Title("Stocks"))
    .mark_line()
    .encode(x="date:T", y="price:Q", color="symbol:N")
)
lines.interactive(bind_y=False)

That works! But I also see another approach. Here you define a custom_theme from the ThemeConfig

from altair.typing import ThemeConfig, theme

custom_theme = ThemeConfig(
    config=theme.ConfigKwds(
        axis=theme.AxisConfigKwds(grid=True, labelFont="system-ui"),
        circle=theme.MarkConfigKwds(fill="purple"),
    )
)

custom_theme

How do I register this as theme?

I searched for register_theme within the altair documentation page, but nothing was returned https://altair-viz.github.io/search.html?q=register_theme. A brief description to the documentation page would help the average user a lot here.

Last, after I enabled the custom theme, how can I de-register this custom theme and go back to the default settings?

Thank you for your patience!

dangotbanned commented 2 months ago

Thanks for your response @mattijn!

I'm going to try my best to address your concerns in a few follow-up comments.

Just finished writing up the first (of 2 or 3), so please know there is more to come

dangotbanned commented 2 months ago

Why altair.typing

@mattijn

I still cannot understand why this ThemeConfig feature is available within the altair.types. I thought we declared that altair.types is the interface of public types within altair. But now we are placing features such as ThemeConfig in there. Is it a type? Therefore it does not help me with discoverability either.

So in the description I provided a link that can clear this up:

This PR aims to improve the UX when authoring a theme. By utilising TypedDict(s) ...

I've picked out some key quotes, but I would really recommend reading the section Using TypedDict Types as this illustrates how TypedDict(s) differ from regular classes:

  • The created TypedDict type object is not a real class object.
  • When called, the TypedDict type object returns an ordinary dictionary object at runtime
  • In particular, TypedDict type objects cannot be used in isinstance() tests such as isinstance(d, Movie).

Back to your question:

Is it a type?

I'm afraid this is complex question to answer, but I will do my best to summarize.

The short (but seemingly a non)answer is that it is not a real class.

I believe that if we were to add these TypedDict(s) to the top-level, that it could cause confusion for users that (understandably) expect them to work the same as SchemaBase classes.

Alternatives

In https://github.com/vega/altair/pull/3536#discussion_r1759328893 I mentioned some related issues, but can now see I forgot to elaborate on what I would prefer for v6.

I would like to have used altair.theme as the namespace instead of altair.typing.theme. However, this would be very easy to mistake for altair.themes - which is a variable storing a registry of themes.

For v6 I would propose the following api changes - and if there is any support I will open a new issue:

This would consolidate all theme related functionality behind a single namespace:

from altair import theme

@theme.register("custom 1", enable=False)
def custom_theme() -> theme.ThemeConfig:
    return {
        "autosize": {"contains": "content", "resize": True}, 
        "config": theme.ConfigKwds(
            circle=theme.MarkConfigKwds(fill="purple")
        ),
    }

...

theme.enable("custom 1")
...
theme.enable("default")

...
dangotbanned commented 2 months ago

Screenshots vs Code blocks

@mattijn

Also, as a user, I really prefer a feature explanation that I can copy and paste. I don't see that in this PR. I see a few screenshots, but that is really not the advocated route for a Minimal, Reproducible Example. For me it is still not clear how I can use this from the PR description.

I have now updated the PR description with collapsible code blocks to accompany each screenshot.

I would absolutely agree that providing a Minimal, Reproducible Example is helpful, although I thought that related more to bugs/issues?

There were a few reasons leaned towards screenshots:

  1. (Most importantly) the key benefits to this PR are seen in an IDE - not at runtime:
    • Autocompletion for dict keys
    • Static type checking feedback
    • Documentation
  2. I had already provided a comprehensive test suite containing code for 13 theme definitions when asked to update the description
    • I seem to have misinterpreted your request as wanting something more than copy/pasting these tests
    • I did mention them and provide a link before presenting the screenshots

      Using the classes themselves is entirely optional, as can be seen in test_theme.py.

  3. Most other typing-focused PRs have also used screenshots to communicate the impact
dangotbanned commented 2 months ago

@alt.register_theme

@mattijn

How do I register this as theme?

I've updated the PR description, this is the example under the heading Options for imports & runtime typing

I searched for register_theme within the altair documentation page, but nothing was returned altair-viz.github.io/search.html?q=register_theme. A brief description to the documentation page would help the average user a lot here.

Well spotted! I wasn't aware this hadn't been included in the API reference yet, but will open a new issue/PR to address it - since that is orthogonal to this PR.

I was waiting until this PR was finished before introducing @register_theme to the User Guide, since they complement eachother well.

There are a few other changes I've noted for the User Guide in https://github.com/vega/altair/issues/3519, including ThemeConfig as mentioned in https://github.com/vega/altair/pull/3536#issuecomment-2353877227

Last, after I enabled the custom theme, how can I de-register this custom theme and go back to the default settings?

Exactly the same way as documented in https://altair-viz.github.io/user_guide/customization.html#defining-a-custom-theme

In https://github.com/vega/altair/pull/3526 I've described this as a convenience for the common case where you use a single theme. That isn't the only possible use-case, but it is the motivating one

dangotbanned commented 2 months ago

I didn't mean to trigger a merge to main with https://github.com/vega/altair/commit/712680b2965eede4c40d93f52d3f963c5d181587

I was trying to fix the conflicts but now I see the comment I'd mentioned was unresolved in https://github.com/vega/altair/pull/3536#issuecomment-2357877156 has been resolved

@binste can we revert this so that @mattijn has time to respond please?

Edit

See https://github.com/vega/altair/pull/3536#issuecomment-2367591832

dangotbanned commented 2 months ago

Thanks @mattijn for https://github.com/vega/altair/commit/3d97faeeb53141cffb1a5bacaa90018982414090

I'm not sure what would be the best way to proceed here, should I open a new PR?

mattijn commented 2 months ago

Thanks for the added explanations! Appreciated. I like the suggestion you provide in https://github.com/vega/altair/pull/3536#issuecomment-2366736230 to consolidate everything under a single namespace, including the proposed ThemeConfig. I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes? For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Thank you for the added explanations regarding the type of ThemeConfig! I currently don't think it needs to be registered as part of interface of altair its public types. The current _TypedDictMeta type seems fine and even if it later becomes something like TypeForm, it's essentially a dictionary (great!) at runtime if I understand correctly.

Also, much appreciated for including both code blocks and screenshots! Great combination as such!

mattijn commented 2 months ago

I don't think it is possible to re-open a merged PR, even when the changes are reverted with the latest commit, probably the best option is to open a new PR😔

dangotbanned commented 2 months ago

Thanks for the added explanations! Appreciated. I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed ThemeConfig. I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

Thank you for the added explanations regarding the type of ThemeConfig! I currently don't think it needs to be registered as part of interface of altair its public types. The current _TypedDictMeta type seems fine and even if it later becomes something like TypeForm, it's essentially a dictionary (great!) at runtime if I understand correctly.

Also, much appreciated for including both code blocks and screenshots! Great combination as such!

Thanks @mattijn

However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes? For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Okay so rather than fully revert this PR, I think the changes we want are strictly concerning a small refactor of the public interface.

3526 was merged after the last release, so renaming + moving @register_theme is all good

I propose:

How does that sound to you?

mattijn commented 2 months ago

The changes requested were only concerning a small refactor of the public interface. Proposal sounds good!