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

docstrings for new introduced methods regarding params and selection_* #2692

Closed
mattijn opened this issue Oct 13, 2022 · 11 comments
Closed

docstrings for new introduced methods regarding params and selection_* #2692

mattijn opened this issue Oct 13, 2022 · 11 comments

Comments

@mattijn
Copy link
Contributor

mattijn commented Oct 13, 2022

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?

@joelostblom
Copy link
Contributor

joelostblom commented Oct 13, 2022

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).

@ChristopherDavisUCI
Copy link
Contributor

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

@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?

@mattijn
Copy link
Contributor Author

mattijn commented Feb 15, 2023

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

mattijn commented Feb 15, 2023

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.

@ChristopherDavisUCI
Copy link
Contributor

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

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).

@mattijn
Copy link
Contributor Author

mattijn commented Feb 19, 2023

By hand is fine.

@ChristopherDavisUCI
Copy link
Contributor

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.

@mattijn
Copy link
Contributor Author

mattijn commented Feb 20, 2023

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.

@ChristopherDavisUCI
Copy link
Contributor

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

@mattijn mattijn closed this as completed Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants