search

vega / altair

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

Consolidate: `alt.theme(s)`, `alt.typing.theme`, `alt.vegalite.v5.theme`, `alt.utils.theme` #3610

Closed dangotbanned closed 1 week ago

dangotbanned commented 1 month ago

What is your suggestion?

The following related PRs have been merged since v5.4.1 release:

This issue is the continuation of the discussion below:

Originally posted by @dangotbanned in post review comment ... 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: - `altair.typing.theme` -> `altair.theme` - Including dropping `altair.typing.ThemeConfig`, which is already included in the above - `@altair.register_theme` -> `@altair.theme.register` - New function: `altair.theme.enable` - Remove `altair.themes` top-level export This would consolidate all **theme** related functionality behind a single namespace: ```py 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") ```
Response by @mattijn in comment ... 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?

Proposing this issue itself

Also tagging @binste, as our discussion in another issue probably needs consideration

Notes

Edit

@mattijn apologies for the delay, but see https://github.com/vega/altair/issues/3610#issuecomment-2374650764 for my thoughts

dangotbanned commented 1 month ago

alt.themes

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?

@mattijn putting aesthetics aside, I see two key issues with using alt.themes.

Instance vs module

As I mentioned in https://github.com/vega/altair/pull/3536#discussion_r1759328893 and https://github.com/vega/altair/pull/3536#issuecomment-2366736230 alt.themes is an instance of ThemeRegistry and PluginRegistry. We could support usage like below, in a few different ways:

How? - Adding a `ThemeRegistry.__getattr__` - Adding the classes as properties of `ThemeRegistry` - Patching the classes onto the instance `themes` 
from altair import themes

themes.register
themes.ThemeConfig
themes.MarkConfigKwds
...

But unless we get particularly creative with importlib, we couldn't support:

from altair.themes import ThemeConfig, MarkConfigKwds, ...

I imagine if we treated themes like it is a module, people would try this to reduce verbosity and be surprised by the import error.

Making alt.themes a module?

Would solve the import issues.

I think we could recreate most of the existing behavior, using a mixture of functions and a module-level __getattr__.

The two things I can't see a solution for are:

alt.themes.register

The inherited signature uses None to unregister a plugin:

PluginRegistry.register() https://github.com/vega/altair/blob/02e8c3479db0bc4b5adc52bb3453eb5d5b46f06a/altair/utils/plugin_registry.py#L135-L147

Since this is not a default, we could probably work around this by adding some sentinel as a default for @alt.themes.register(). The source for @dataclasses.dataclass() has an example of dual usage (function and decorator).

I'm fairly sure the issues around this wouldn't be the technical aspect, mostly just a bit of extra complexity for the user and more explanation in the docs.

alt.theme

I really do think it would be simpler to start fresh with a new namespace, without any additional baggage.

We could also be sure that this wouldn't change the meaning of existing user code. A deprecation warning can always be ignored for those determined to still use alt.themes

mattijn commented 1 month ago

Thanks for the detailed description! I’m good with your suggestion to start with alt.theme. While it may not become or serve as a theme or plugin registry, I’m still hopeful we can achieve a similar user experience to the current alt.themes (even if it’s just wishful thinking😄)

dangotbanned commented 1 month ago

Thanks for the detailed description! I’m good with your suggestion to start with alt.theme. While it may not become or serve as a theme or plugin registry, I’m still hopeful we can achieve a similar user experience to the current alt.themes (even if it’s just wishful thinking😄)

No problem @mattijn, thanks I'll try to work on this soon.

We can always have themes accessible via alt.theme.themes - if you'd see a benefit?

mattijn commented 1 month ago

Based on your work in PR #3618 (approaching from a fresh perspective, without considering legacy code) and your findings in issue #3586, could PR #3618 potentially serve as a drop-in replacement for the current alt.themes? What functionality would we lose, and would that actually impact detected real-world use cases (as found by sourcegraph)?

dangotbanned commented 1 month ago

Based on your work in PR #3618 (approaching from a fresh perspective, without considering legacy code) and your findings in issue #3586, could PR #3618 potentially serve as a drop-in replacement for the current alt.themes? What functionality would we lose, and would that actually impact detected real-world use cases (as found by sourcegraph)?

Thanks @mattijn

New search for all usage of alt.themes. sourcegraph got 45 results.

Note

I'll try to put together a short analysis/visualization of this usage soon

3618 Functional Changes

Currently no direct alt.theme.___ replacement

These can still be accessed via alt.theme.themes.___.

It would probably be possible to add these, but since they use @property it may be more difficult to document them.

Update

By the end of writing this I'd talked myself into adding support for alt.themes.(active|options) See https://github.com/vega/altair/pull/3618#discussion_r1781548105 for updated preview of API Reference

Diffs showing these changes applied internally

dangotbanned commented 1 month ago

@mattijn I'm thinking about making alt.theme.themes -> alt.theme._themes.

As of https://github.com/vega/altair/pull/3618#commits-pushed-58da996 everything has a replacement in alt.theme already.

With the original registry still accessible (w/ deprecation alt.themes) - I can't see having it available here as well being anything more than a point of confusion