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

Enhance converter docstring and keyword definitions #1481

Open
1 task done
maximlt opened this issue Jan 27, 2025 · 2 comments
Open
1 task done

Enhance converter docstring and keyword definitions #1481

maximlt opened this issue Jan 27, 2025 · 2 comments
Milestone

Comments

@maximlt
Copy link
Member

maximlt commented Jan 27, 2025

Comparing the keyword documented in the signature and defined in the docstring of the HoloViewsConverter class:

  • Keywords in the signature but not in the docstring:
backlog
by
col
data
debug
fields
framewise
group_label
groupby
kind
kwds
label
persist
precompute
row
stacked
stream
subplots
use_dask
use_index
value_label
x
y
  • Keywords in the docstring but not in the signature (EDIT: see below, no immediate need to change that):
bgcolor
clabel
fontscale
fontsize
frame_height
frame_width
logz
max_height
max_width
min_height
min_width
padding
responsive
shared_axes
width
xaxis
xformatter
xlabel
xticks
yaxis
yformatter
ylabel
yticks
@maximlt
Copy link
Member Author

maximlt commented Feb 20, 2025

Keywords in the docstring but not in the signature:

Thinking a bit more about this, I don't think there's a need to add all the keywords to the signature, it'll make it crazier than it is and we'd have to deal with default values, as now most of these keywords are passed downstream only when they're passed to the converter.

Instead, one issue we seem to have is that, while the Converter class defines lists of keywords (_axis_options, _data_options, _op_options, etc.), it looks like these lists don't contain all the keywords available. For example:

  • flip_xaxis is in the signature, in the docstring, but not in _axis_options
  • precompute is in the signature, not in the docstring, and not in _data_options.

These lists are used for different purposes, including dynamically building the signature (e.g. useful in IPython) and for warning the user when they pass some unexpected keyword.

To build the Plotting Options Reference, we'll need to have a way to dynamically collect these keywords, their docstring, the group they belong to, and even maybe their type hints. To get there, I think the first step is to ensure we have the right data:

  • Ensure all the generic keywords are listed in the special lists correctly (it might be some are wrongly listed too)

There are also a few cases of keywords that are not part of the signature, not in the docstring, and not in any of these special lists. One way to find them is just to go through the code of the Converter class and spot things like kwds.get(<keyword>, <default>). One such example that comes to mind is max_px, there might be more.

  • Ensure all the keywords caught internally are all documented and listed

To make sure this all works well long-term:

  • Write tests that check that the keywords are listed and documented

@maximlt maximlt changed the title Enhance converter docstring and signature Enhance converter docstring and keyword definitions Feb 20, 2025
@maximlt
Copy link
Member Author

maximlt commented Feb 20, 2025

In the future we could maybe leverage TypedDict to type all these keywords. It looks like it'd be useful if https://peps.python.org/pep-0728/ was accepted.

import typing as t

class AxisOptions(t.TypedDict, total=False):
    height: int
    width: int
    logx: bool

class GeoOptions(t.TypedDict, total=False):
    geo: bool
    project: bool


class AllOptions(AxisOptions, GeoOptions):
    pass

print(AxisOptions.__optional_keys__)
print(AxisOptions.__annotations__)
# frozenset({'logx', 'width', 'height'})
# {'height': <class 'int'>, 'width': <class 'int'>, 'logx': <class 'bool'>}

def plot(
    **kwargs: t.Unpack[AllOptions],
):
    height = kwargs['height']
    geo = kwargs['geo']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

2 participants