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

Promote the use of alt.when().then().otherwise() over alt.condition() in the User Guide? #3500

Open
joelostblom opened this issue Jul 24, 2024 · 11 comments · May be fixed by #3544
Open

Promote the use of alt.when().then().otherwise() over alt.condition() in the User Guide? #3500

joelostblom opened this issue Jul 24, 2024 · 11 comments · May be fixed by #3544

Comments

@joelostblom
Copy link
Contributor

What is your suggestion?

After all the fantastic work of @dangotbanned , we now have alt.when().then().otherwise() as an alternative to alt.condition(condition, if_true, if_false). This new syntax allows for multiple predicates to be specified, which is not possible with alt.condition, looks more idiomatic (in line with our method chaining for encoding channel options), and is similar in length to type to alt.condition (but might be shorter in the future if it becomes possible to pass string literals in an unambiguous manner).

When we introduced the method based syntax for channel options, we updated the docs everywhere and added a note in https://altair-viz.github.io/user_guide/encodings/index.html#channel-options

image

Should we do the same for alt.when().then().otherwise() or are there any reasons to keep alt.condition as the main conditional syntax in the docs?

Have you considered any alternative solutions?

If we keep alt.condition the favored option in the documentation, we should at least add a section on how to use alt.when... in the User Guide.

Regardless of how we decide to update the docs, I think we should also explicitly mention the use of empty inside the conditional, as opposed to in the parameter definition (#3490).

@dangotbanned
Copy link
Member

Thanks for the shoutout @joelostblom

Only thing I wanted to add is I'm currently working on fleshing out the examples in #3492

There is quite a lot of functionality packed into the new methods, and from experience with polars.when I found it really quick to pick up learning this way

@mattijn
Copy link
Contributor

mattijn commented Jul 25, 2024

@dangotbanned
Copy link
Member

dangotbanned commented Jul 26, 2024

and is similar in length to type to alt.condition (but might be shorter in the future if it becomes possible to pass string literals in an unambiguous manner).

@joelostblom the proposal here #3476 (comment) I see as helping get towards these being equal:

alt.when(predicate).then(alt.value("red")).otherwise(alt.value("blue"))
alt.when(predicate).then("red").otherwise("blue")

The second half of this comment shows the issues with assuming shorthand, but that could be resolved by requiring alt.[agg|field]() to opt-out of auto alt.value()

Long-term goal, but maybe something for the next breaking release

@dangotbanned
Copy link
Member

@joelostblom @mattijn

Do you have any specific ideas on how to approach this?

If we keep alt.condition the favored option in the documentation, we should at least add a section on how to use alt.when... in the User Guide.

I think the Examples section for alt.when are a good starting point for this.

I also left this comment that explains alt.when in terms of alt.condition.
Adapting that might be useful?

@joelostblom
Copy link
Contributor Author

joelostblom commented Aug 18, 2024

A few thoughts here:

  1. I would be in favor of replacing the existing occurrences of alt.condition in the User Guide with alt.when().then() and mention in a callout box that this new syntax was added in 5.4.0 similar to what we did for Channel Options in the screenshot above.
  2. The examples you have already added are really helpful, and I would like those to be in the User Guide as well. I'm unsure if we should put them directly into the interactivity page of if we should have them in a subpage focused on more advanced condition syntax. It would be helpful if this also included the use of empty inside the conditional.
  3. Echoing what I wrote in Clarifications to the interactive docs #3362 (comment) (although this could be a separate PR)

    The interactive page is becoming quite long so we might consider splitting it into two (or three if we move the JupyterChart section under "interactivity" too)? The bindings and widgets page is about half page so that could be a natural split point, but I don't know if that will break some links; then it's probably not worth it.

  4. As in (1), I think the new syntax should also be favored in the gallery examples. I don't think we need to do what we did with the Channel options here and have tabs showing both the new and old syntax, just the new one is enough.

@dangotbanned dangotbanned linked a pull request Aug 18, 2024 that will close this issue
28 tasks
@dangotbanned
Copy link
Member

dangotbanned commented Aug 18, 2024

@joelostblom really appreciate the detailed response!

  1. I would be in favor of replacing the existing occurrences of alt.condition in the User Guide with alt.when().then() and mention in a callout box that this new syntax was added in 5.4.0 similar to what we did for Channel Options in the screenshot above.

I agree. I was about to suggest adding the callout at conditions-filters - which I now understand was what @mattijn meant in #3500 (comment)


  1. The examples you have already added are really helpful, and I would like those to be in the User Guide as well.

Have a few thoughts on this.
For reference, @mattijn added screenshots of the output in #3492 (review).

If at all possible, having sphinx apply the .. altair-plot:: directive to altair.when during docbuild would improve what is already there.
Maybe there is a way around this, but I'd want to avoid it being part of the docstring:

Screenshot

image

It would be helpful if this also included the use of empty inside the conditional.

So in #3485 I added interactive_bar_select_highlight which has an example of this usage.
What do you think about linking to that in the docstring?
I'm hesitant to repeat it, due to the added length - while also not being able to see the output.

It was just recreating a vega-lite example, so I'm still open to a shorter example that illustrates the point


  1. Echoing what I wrote in Clarifications to the interactive docs #3362 (comment) (although this could be a separate PR)
    > The interactive page is becoming quite long so we might consider splitting it into two (or three if we move the JupyterChart section under "interactivity" too)? The bindings and widgets page is about half page so that could be a natural split point, but I don't know if that will break some links; then it's probably not worth it.

This somewhat overlaps with (2), so if that introduces a subpage that might change my opinion here.
Definitely think it would be helpful to split up interactive-charts.
My suggestion for this would be (inclusive bounds):

  1. Parameters - Parameter Composition
  2. Expressions
  3. Bindings & Widgets
  4. Access Params from Python - JupyterChart

I think the reordering would be helpful, so that expressions are introduced before being used in data-driven-comparisons and encoding-channel-binding.


  1. As in (1), I think the new syntax should also be favored in the gallery examples. I don't think we need to do what we did with the Channel options here and have tabs showing both the new and old syntax, just the new one is enough.

100% agree, if anything adding a tab for that would make it more difficult to understand

@joelostblom
Copy link
Contributor Author

(sorry this became longer than intended)

2. I generally prefer to include examples in the User Guide if there is a suitable location for them. I think this is one such case, so I would vote for moving the gallery example into the new section of the user guide instead.

Your point about using the altair-plot directive on the docstring examples got me thinking of where would be the ideal place to have example code. So far we have mostly included it in the User Guide and Gallery, but not really in the docstrings. Having said that, I think it is helpful to see at least some simple examples directly in the docstring, but also that we are consistent so that users know where they can find the example code. It also seems redundant to have the same code appear twice; both in the User Guide and in the docstring/API reference.

I think we can decide to either lean into the docstring examples, add them to all objects that don't have them currently, and then inline the docstring examples in the relevant sections of the User Guide. Pros: easy to view example right in the docstring, only update in one place (although some User Guide sections are too long to include in the respective object's docstring: e.g. the section on mark_geoshape). Cons: Chart don't show up in docstrings, would have to add examples to a fair amount of objects (can we even do it for what we autogenerate from Vega-Lite such as the marks, transform, etc?), would we have the same

Another option is to focus on having the example code only in the User Guide (maybe with a link in the docstring to the relevant section in the docs?). Pros: Closer to the existing approach so there is less to update, charts only show up online anyways, Cons: Not possible to view example directly in the help pop up in IDEs.

A third approach would be something in between with just a simple example in the docstring and more elaboration in the User Guide. Pros: There is both something directly in the docstring and also an elaborate version in the User Guide. Cons: We're now updating in two places, but maybe the docstring example can be kep so simple that it doesn't matter much.

There is also a larger discussion around what goes into the Gallery and what doesn't. We don't want it too cluttered, but it is also a popular page to visit. We could probably remove some examples from the Gallary and also add links from the Gallery to the relevant sections in the User Guide so that people find them too (but this is a separate PR).

3. I vote that we split the interactive docs and introduce the examples you have created for alt.when as a sub-heading under "Conditions & Filters" (maybe also expanding the narration of that page just a bit).

The reordering of 2 and 3 is not intuitive for me as we make quite heavy use of widgets when explaining the expressions (and I don't see us using expressions much in the widget section). Don't you think they would need to see widgets before understanding the expression section?

@dangotbanned
Copy link
Member

@joelostblom really sorry for missing #3500 (comment), I seem to have dismissed the notification via email? 🤦‍♂️

Will do my best to get back this soon, but thank you for taking the time to write it!

@dangotbanned
Copy link
Member

dangotbanned commented Aug 27, 2024

@joelostblom finally got back to this.

ideal place to have example code. So far we have mostly included it in the User Guide and Gallery, but not really in the docstrings. Having said that, I think it is helpful to see at least some simple examples directly in the docstring, but also that we are consistent so that users know where they can find the example code.

I've seen diataxis mentioned in altair before (#3117), which might be helpful in achieving consistency


would have to add examples to a fair amount of objects (can we even do it for what we autogenerate from Vega-Lite such as the marks, transform, etc?), would we have the same

So I definitely appreciate examples in docstrings, but would be hesitant in adding them to autogenerated docstrings.
They are already very long and repetitive, adding examples into the mix I think would be overwhelming.

Also if they are not written by hand, I expect they will be of low utility
- E.g. just demonstrating which values are allowed
- Rather than showing examples of why you might pass specific arguments


A third approach would be something in between with just a simple example in the docstring and more elaboration in the User Guide. Pros: There is both something directly in the docstring and also an elaborate version in the User Guide. Cons: We're now updating in two places, but maybe the docstring example can be kep so simple that it doesn't matter much.

Of the options presented, this would be my preference with ...

Additional Proposal

I'm viewing examples less in terms of simple vs complex, and more in terms of serving the context they are displayed in.
Of course these may overlap to some degree, but I think the needs differ enough to naturally lead to different examples written:

Docstrings/API Reference

  • Showing you how the object/function is used
  • Focused on utility, minimal discussion

User Guide

  • Showing you how the altair API fits together
  • Higher level, discussing why you'd reach for certain tools
  • Doesn't go through every possible permutation

Gallery

  • Showing how to produce specific charts
  • Currently these are mostly short description, some have inline comments
  • Goal-oriented

  1. I vote that we split the interactive docs and introduce the examples you have created for alt.when as a sub-heading under "Conditions & Filters" (maybe also expanding the narration of that page just a bit).

If you have the time, would you be able to make the split (either as its own PR or in #3544)?

I am happy to do the alt.when parts, but the topic of documentation seems to be something you have clear ideas on and I wouldn't want to make any naive changes


The reordering of 2 and 3 is not intuitive for me as we make quite heavy use of widgets when explaining the expressions (and I don't see us using expressions much in the widget section). Don't you think they would need to see widgets before understanding the expression section?

I'll defer to you on this as I haven't used expressions in much detail and you seem much more knowledgeable on them than I.

@dangotbanned
Copy link
Member

Finally found waterfall chart example.

I recalled seeing this comment but couldn't find it when searching:

alt.condition does not support multiple if else conditions which is why
we use a dictionary instead. See https://stackoverflow.com/a/66109641
for more information

This should be re-written with alt.when and removing the comment, which is no longer as useful

@joelostblom
Copy link
Contributor Author

joelostblom commented Aug 27, 2024

I've seen diataxis mentioned in altair before (#3117), which might be helpful in achieving consistency

Good reminder! I also think this could be helpful in the larger picture, but it's a bigger project for me to understand diataxis more intimately and think about if it would serve us to (iteratively) move towards that. But I had a glance at the page and as per https://diataxis.fr/compass/ and https://diataxis.fr/complex-hierarchies/ maybe our current docs map onto diataxis something like this (mostly for future reference):

Diataxis Altair
Tutorial User guide
How to guide Getting started + Gallery
Reference API reference
Explanation Advanced usage + a little bit here and there

I also think it would be helpful with something simple like more (two-way) links between the User Guide and the Gallery examples to guide readers toward more relevant information.

So I definitely appreciate examples in docstrings, but would be hesitant in adding them to autogenerated docstrings. They are already very long and repetitive, adding examples into the mix I think would be overwhelming.

I agree with this.

...and more in terms of serving the context they are displayed in.

I find this framing and the structure in your additional proposal helpful. Let's proceed with that framework as our guide for deciding where to put examples and what they should look like.

If you have the time, would you be able to make the split (either as its own PR or in #3544)?

Yup I can put up a separate PR.

This should be re-written with alt.when and removing the comment, which is no longer as useful

Agree! I can also update https://stackoverflow.com/questions/66108224/combine-hover-and-click-selections-in-altair/66109641#66109641 once there is a doc section to link too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants