Aliases for ImputeParams and TitleParams

[ChristopherDavisUCI]

Let's not merge this for a little while to make sure this doesn't break anything and to think about whether we need to do anything more sophisticated. The point is to make it easier for impute and title to find the appropriate docstrings, as discussed in #2592

[joelostblom]

And I would add also make it more convenient and consistent to use these classes since the syntax is briefer and more similar to that of the other classes we use for similar things, e.g. alt.Title instead of alt.TitleParams just like we use alt.X etc

@mattijn do you have any thoughts there?

[mattijn]

There are more candidate aliases I think?

import requests as r
import re
vl_scheme = r.get('https://vega.github.io/schema/vega-lite/v5.json').json()
scheme_str = str(vl_scheme['definitions'])
col = []
for word in re.findall(r"\w+", scheme_str):
    if 'Params' in word:
        col.append(word)
list(set(col))

Returns 10 candidates:

['TimeUnitParams',
 'TitleParams',
 'SchemeParams',
 'GraticuleParams',
 'SequenceParams',
 'BinParams',
 'ImputeParams',
 'ScaleBinParams',
 'ScaleInterpolateParams',
 'AutoSizeParams']

Not sure about this *Offset. There are many more:

col_offset = []
for word in re.findall(r"\w+", scheme_str):
    if 'Offset' in word:
        col_offset.append(word)
list(set(col_offset))

['theta2Offset',
 'labelOffset',
 'y2Offset',
 'strokeDashOffset',
 'Offset',
 'gridDashOffset',
 'yOffset',
 'tickDashOffset',
 'symbolDashOffset',
 'xOffset',
 'nestedOffsetPaddingInner',
 'StackOffset',
 'bandWithNestedOffsetPaddingInner',
 'thetaOffset',
 'bandWithNestedOffsetPaddingOuter',
 'x2Offset',
 'OffsetDef',
 'strokeOffset',
 'radiusOffset',
 'radius2Offset',
 'domainDashOffset',
 'symbolOffset',
 'tickOffset',
 'gradientLabelOffset',
 'labelFlushOffset',
 'Offsets']

[joelostblom]

Ah, I only looked through some of the encodings methods when I tried so I missed those other Params helper classes, I think it would make sense to add those.

For the Offset, I think StackOffset (and maybe Offset(s)) are different from the rest in that they are classes that can be passed inside the encodings (such as X(stack=StackOffset(...)). Many of the others seem to be names of parameters or encoding channels and would not need an alias I believe.

[ChristopherDavisUCI]

@joelostblom I don't have a clear view of how the docstrings are currently generated. Is this PR still useful? If it is, I will go through the comments above carefully and try to implement the suggestions.

[joelostblom]

These were required in the way I originally implemented the docstrings, but I haven't reviewed if the additions you made changed that. Regardless, I would be in favor of having these shortcuts (at least Title), to make it more consistent and convenient to type them out.

[ChristopherDavisUCI]

Thanks @mattijn for the extra examples above. I looked through them but didn't find any others which were being used as these attribute setter methods, so I didn't make any more additions beyond @joelostblom's suggested StackOffset (which corresponds to the attribute setter method stack).

It seems kind of random to me that the Python 3.8 build is failing but not the builds for Python 3.7, 3.9, 3.10, 3.11. Do any of you see what the issue is?

If the test issue can be resolved I'd say this is ready to be merged from my perspective.

[mattijn]

I'll rerun the failed tests.
Meanwhile can you have a look to this selected code @ChristopherDavisUCI? It was introduced in this PR: #1667. It seems that core.SequenceParams and core.GraticuleParams have its own utility function defined (alt.sequence() and alt.graticule()). Is that something we have to consider here as well?

[ChristopherDavisUCI]

Hi @mattijn, thanks! I don't see any conflict with those two definitions you shared. Is there anything in particular you were worried about with those?

I do think each customization we add that doesn't stem from Vega-Lite is a possible headache down the road, but since these aliases Title, Impute, and Stack are only being used to get docstrings, the risk seems pretty low to me.

Please just let me know if I missed your concern!

[mattijn]

Would it be possible to add some examples what is different after this PR? I thought that we are getting a capital case alt.Title() instead of alt.TitleParams() function (not sure exactly how and when to use these) and was surprised that alt.GraticuleParams is a small case alt.graticule(), but the latter seems to do more. Anyhow, some examples would be beneficial!

[joelostblom]

I suggest that we remove the Stack alias and keep the other two. It would also be good to go through the docs and gallery and replace all the occurrences of TitleParams with Title and all the ImputeParams with Impute, since there is now no reason to use the longer more inconvenient name.

---

Some more detailed thoughts below:

It looks like alt.sequence is a mix of alt.SequenceParams and alt.SequenceGenerator so I think it makes sense that these are lower case functions (and the same for alt.graticule). The changes here are more similar to alt.Bin/alt.BinParams which I don't think has a utility function anywhere.

As for when to use alt.Title, it is helpful when creating titles with formatting or subtitles as in this SO answer https://stackoverflow.com/a/66147967/2166823. alt.Impute has similar advantages since it also takes multiple parameters, but for alt.Stack I don't see an advantage other than for the docstrings since it only takes a single parameter.

I did confirm that these aliases are all required for the propagation of the docstrings from the corresponding helper classes, but the docstring from the StackOffset class is sparse and all the info there is already contained within the section of the docstring that is extracted from alt.X() etc. There is also a discrepancy between the two docstrings where alt.StackOffset does not include true, false, and null although these are all allowed values.

[ChristopherDavisUCI]

Thanks @joelostblom for the detailed explanations! I went ahead and removed the StackOffset alias. My understanding is that the docstrings we're finding are not exhaustive, but typically show the most flexible option. For example, my understanding is that bin will show the BinParams docstring, and I think that will not indicate that bin(True) is a valid option. To recognize that bin(True) is a valid option, you would have to look at the docs for the encoding channel. For example, the docs for alt.X rather than for alt.X().bin. Does that also match your understanding?

I'm a little hesitant to go through changing the docs and examples using these aliases. My understanding is that in the long-run we will probably be using the title method rather than title = alt.Title, so to me it seems easier to wait and get feedback on these attribute setter methods. What do you think?

[ChristopherDavisUCI]

In this most recent commit, I added a test trying to check if we were overwriting anything when we define Bin, Impute, and Title. I don't have much experience writing tests, so please let me know if anything seems strange about it (and, e.g., if I've put it in a reasonable place).

[joelostblom]

Looking good, and I think that test makes sense but I will ping @mattijn since he recently moved the tests around.

Also note that there is one failing test for Python 3.9.

---

my understanding is that bin will show the BinParams docstring, and I think that will not indicate that bin(True) is a valid option. To recognize that bin(True) is a valid option, you would have to look at the docs for the encoding channel. For example, the docs for alt.X rather than for alt.X().bin. Does that also match your understanding?

We actually fetch the docstring from both alt.BinParams and from alt.X and combine them together. The only thing missing from the alt.X docstring is the first line, in this case bin : anyOf(boolean, :class:BinParams, string, None) which I am not sure would be helpful to include since the hint that we can use BinParams is unhelpful now that we can write all the parameter names directly in alt.X().bin(param=) rather than doing alt.X().bin(alt.BinParams(param=)).

The current docstring actually does include a mention of the boolean, just not on top:

We can continue this discussion in #2806

[ChristopherDavisUCI]

Thank you! GitHub is showing me "1 change requested" but as far as I can tell this is now updated to match the requested changes.

[mattijn]

@joelostblom did not yet approve your changes as part of his review

[joelostblom]

LGTM! Thank you!