altair icon indicating copy to clipboard operation
altair copied to clipboard

docstrings for new introduced methods regarding params and selection_*

Open mattijn opened this issue 3 years ago • 1 comments

Currently the docstrings for some new introduced parameter methods are rather limited: image

Where in the above example I've defined it as such:

highlight = alt.selection_point(
    name="hover",
    on="mouseover",
    nearest=True,
    encodings=["x"],
    empty=False,
    clear="mouseout",
)

How can one know about this without docstrings?

mattijn avatar Oct 13 '22 20:10 mattijn

I agree this should be more informative. I see that there are a couple of TODOs in that file on improving the docstrings for the parameters and for the bindings. Maybe we can use the same I approach I used #1629

https://github.com/altair-viz/altair/blob/91ddea9170e3949286daa9889f5e2bd91e3d8249/altair/utils/schemapi.py#L601-L604

to overwrite the docstring of selection_interval with that from the IntervalSelectionConfig class:

https://github.com/altair-viz/altair/blob/b186a76a25b6d7fd0c0efdeb58e0697c1876f813/altair/vegalite/v5/schema/core.py#L6329-L6349

On a related note, is there any use to keep having alt.selection() exposed as a public function when we mostly use alt.selection_point/interval directly? I don't think it offers anything valuable and just leads to ambiguity of which selection function to use, but I might be missing something. An alternative would be to only keep the former and always use alt.selection('point', ...) etc, but I think that is less common currently and in the docs (although more in line with VL).

joelostblom avatar Oct 13 '22 22:10 joelostblom

Is it possible some of these issues have been incidentally fixed along the way? For example, I seem to have docstrings showing up okay.

Screenshot 2023-02-15 at 11 17 57 AM

shift+tab also seems to reveal a docstring (I can't get a screenshot at the moment).

A different issue is that these are the docstrings for the lower-level objects, such as core.IntervalSelectionConfig, so for example, name is not mentioned, even though name is an expected keyword argument for our higher-level Altair function.

Before looking into this further, I wanted to check @mattijn to what extent the issue you reported in October has been resolved. Thanks!

ChristopherDavisUCI avatar Feb 15 '23 19:02 ChristopherDavisUCI

@joelostblom I agree it could make sense to hide alt.section, so that alt.selection_point and alt.selection_interval are encouraged. Do you remember how to do that?

ChristopherDavisUCI avatar Feb 15 '23 19:02 ChristopherDavisUCI

You are right! This is fixed along the way. Great. image

If you want to hide the alt.selection from the autocomplete, you'll have to assign alt.selection a deprecated status around here https://github.com/altair-viz/altair/blob/master/altair/vegalite/v5/api.py#L372 by adding:

@utils.deprecation.deprecated(
    message="'selection' is deprecated.  Use xxx"
)
def selection(type=Undefined, **kwds):

mattijn avatar Feb 15 '23 20:02 mattijn

Hm, not sure. This is the current docstring for alt.param, https://github.com/altair-viz/altair/blob/master/altair/vegalite/v5/api.py#L303-L320.

def param(name=None, select=None, **kwds):
    """Create a named parameter.
    Parameters
    ----------
    name : string (optional)
        The name of the parameter. If not specified, a unique name will be
        created.
    **kwds :
        additional keywords will be used to construct a parameter.  If 'select'
        is among the keywords, then a SelectionParameter will be created.
        Otherwise, a VariableParameter will be created.
    Returns
    -------
    parameter: Parameter
        The parameter object that can be used in chart creation.
    """

I would say that is too limited to consider this issue complete.

mattijn avatar Feb 15 '23 20:02 mattijn

Glad the docstrings also working on your end @mattijn! The displayed signature isn't perfect, because on the Altair side we allow a parameter and its selection configuration to be defined simultaneously, but the above docstring only mentions the selection portion. For example, I believe all of these are missing from selection_interval: {"name", "value", "bind", "empty", "init", "views"}.

I agree that we should keep this issue open for now.

ChristopherDavisUCI avatar Feb 15 '23 20:02 ChristopherDavisUCI

Hi @mattijn, I will try to improve the parameter docstrings. Is there anything wrong with just typing it out by hand in VS Code, including for example the valid keyword arguments? I just want to make sure that's reasonable before getting started.

Getting the valid arguments automatically from Vega-Lite is harder than usual for parameters, because on the Vega-Lite side, there is a parameter definition like this

{
  "name": "brush",
  "select": {"type": "interval", "encodings": ["x"]}
}

but on the Altair side, those values of "name" and "encodings" would typically be passed to the same function (like alt.selection_interval).

ChristopherDavisUCI avatar Feb 19 '23 16:02 ChristopherDavisUCI

By hand is fine.

mattijn avatar Feb 19 '23 16:02 mattijn

Is there a rule of thumb for when Undefined should be used as a default value, and when None should be used as a default value? I'm increasing the number of keyword arguments to alt.param (for example, bind and value), etc, but am finding myself just guessing whether to use None or Undefined as the default values.

ChristopherDavisUCI avatar Feb 19 '23 22:02 ChristopherDavisUCI

In the context of Altair, None, serializes to null in JavaScript, sometimes that is desired. Again, in the context of Altair, you probably want Undefined most of the time.

mattijn avatar Feb 20 '23 06:02 mattijn

Thanks @mattijn, please see if #2908 addresses this issue.

ChristopherDavisUCI avatar Feb 20 '23 15:02 ChristopherDavisUCI