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

docs: Promote when-then-otherwise in User Guide #3544

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Aug 18, 2024

Will close #3500

Status

All outstanding work should be scoped to parameters.rst.
Corrections/suggestions elsewhere are still welcome

The linked version represents Draft 1.

Tasks

  • Use alt.when in waterfall chart
  • Fill out tasks following discussion
  • Add callout re: v5.4.0 change comment
    • Probably makes most sense here
      parameters.rst
      Conditions & Filters
      ~~~~~~~~~~~~~~~~~~~~
      Conditional Encodings
      ^^^^^^^^^^^^^^^^^^^^^
      The example above is neat, but the selection interval doesn't actually *do* anything yet.
  • when(..., empty=False) example in User Guide comment
  • parameters.rst
    • Draft 1
    • Draft 2
    • Draft N
  • Replace condition references with when comment
    • User Guide
      • Interactions
        • bindings_widgets.rst
        • expressions.rst
        • jupyter_chart.rst
      • Marks
        • geoshape.rst
        • line.rst
        • text.rst
      • Transform
        • filter.rst
        • pivot.rst
      • compound_charts.rst
      • times_and_dates.rst
    • Case Studies
      • exploring-weather.rst
    • README.md
    • Gallery
      • examples_methods_syntax/*.py
      • examples_arguments_syntax/*.py

No visual change, but makes the markup itself easier to read
- Replaced `dict` with `when-then-otherwise`
- Replaced string expressions with `python` equivalent
- Formatting update to avoid horizontal overflow
@dangotbanned
Copy link
Member Author

@joelostblom since your mention of this PR in #3548 (reply in thread), I've circled back to this.

If you have the time, would you be able to double-check the tasks I've added in the description please?

Going to start working through them now

@joelostblom
Copy link
Contributor

Looks great! The location for the callout seems suitable to me. The only thing I would add is that I think we should include an example in the user guide section for the use of empty inside the conditional, as opposed to in the parameter definition (#3490).

@dangotbanned
Copy link
Member Author

Looks great! The location for the callout seems suitable to me. The only thing I would add is that I think we should include an example in the user guide section for the use of empty inside the conditional, as opposed to in the parameter definition (#3490).

Perfect thanks @joelostblom, I've added that to the list.

Managed to get through a good chunk today, I'm expecting parameters.rst will require the most work since it calls out alt.condition directly

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a peak and it is looking great so far! One comment regarding syntax, curious to hear what you think.

doc/case_studies/exploring-weather.rst Show resolved Hide resolved
First draft version. Unlike the other commits, this has to introduce the concept - so will require more scrutiny.

vega#3544 (comment)
@joelostblom
Copy link
Contributor

@dangotbanned I noticed that you tagged me for review here, but also that the PR is still marked as draft, so I just want to double check whether you want a review now or you are waiting to finish something up first.

@dangotbanned
Copy link
Member Author

@dangotbanned I noticed that you tagged me for review here, but also that the PR is still marked as draft, so I just want to double check whether you want a review now or you are waiting to finish something up first.

Thanks for checking in @joelostblom!

I was hoping to get feedback focused on the Status section of the description.

Do you think I should move this out of draft - to make it clearer that I'm interested in a review?

@dangotbanned dangotbanned marked this pull request as ready for review September 10, 2024 06:38
Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience @dangotbanned and sorry for not getting here earlier. I had a quick read in the docs and think this is looking great already!

I think it would be instructor to include the following examples in a subsection at the end of "Conditions and Filters":

  • Simple conditions may be expressed without defining a default (maybe with a simpler example than invalid=None)
  • Chain calls to express precise queries
  • Reuse defined when variable

Whereas the following seem more like "another way of doing something we already have shown" and I would leave them out of the User Guide (maybe a link a the bottom of the new subsection mentioning there are even more example in the when API reference?)

  • Predicates passed as positional arguments will be reduced with & (since this is just an alternative way of achieving something)
  • Using keyword-argument constraints can simplify compositions (since this is only for AND and ==)

doc/user_guide/interactions/parameters.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions/parameters.rst Outdated Show resolved Hide resolved
@dangotbanned dangotbanned marked this pull request as draft September 18, 2024 10:26
All of the charts using this function were silently producing no output.
`brush` had been defined globally in an earlier directive

vega#3544 (comment)
@dangotbanned
Copy link
Member Author

Thanks for your patience @dangotbanned and sorry for not getting here earlier. I had a quick read in the docs and think this is looking great already!

No worries @joelostblom, thanks for taking the time to review

I think it would be instructor to include the following examples in a subsection at the end of "Conditions and Filters":

  • Simple conditions may be expressed without defining a default (maybe with a simpler example than invalid=None)
  • Chain calls to express precise queries
  • Reuse defined when variable

Sounds good to me.
Just to be sure I've understood correctly, which of these locations did you mean?

  1. End of Conditional Encodings
    • The end of the alt.when section of Conditions & Filters
  2. End of Conditions & Filters
    • The actual end of Conditions & Filters

Whereas the following seem more like "another way of doing something we already have shown" and I would leave them out of the User Guide (maybe a link a the bottom of the new subsection mentioning there are even more example in the when API reference?)

  • Predicates passed as positional arguments will be reduced with & (since this is just an alternative way of achieving something)
  • Using keyword-argument constraints can simplify compositions (since this is only for AND and ==)

That makes sense, I definitely want to avoid adding too much here - especially if as you've said there is already a documented way to do it

@joelostblom
Copy link
Contributor

While reviewing 50a520e, I got some ideas for rearranging sections on this page to make it easier to follow. I have most of it done and will finish it off tomorrow to show you my suggestion instead of trying to explain it.

@dangotbanned
Copy link
Member Author

While reviewing 50a520e, I got some ideas for rearranging sections on this page to make it easier to follow. I have most of it done and will finish it off tomorrow to show you my suggestion instead of trying to explain it.

All good with me @joelostblom

Feel free to move this PR out of draft when you're ready.
I was waiting until either I came up with a heading for Untitled More When or you took a look

To be able to fully document the behavior of `when`,
I made a few organizational changes:

- Split "Conditions" and "Filter" since these are two
  different concept anyways.
- I also renamed the first heading to illustratet that
  the three big concepts are "Parameter", "Conditions", and "Filters".
- Named the main new "when" section "Understanding when()"
  and included an example of conditional branches in pseudocode early on
  and a runnable example later.
- I also added an additional subheading for "Linking across charts"
- Included mention of the use of `empty=False` inside `when` together
  with a short pseudocode example.
- Creating a new section at the end about combining parameters,
  which contains both the previous section on parameter composition,
  but also two new subsection on multiple conditions and conditional
  when branches.
- I expanded on the parameter composition examples as well.
Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dangotbanned To be able to fully document the behavior of when,
I made a few organizational changes in e363abd

  • Split "Conditions" and "Filter" since these are two
    different concept anyways.
  • I also renamed the first heading to illustratet that
    the three big concepts are "Parameter", "Conditions", and "Filters".
  • Named the main new "when" section "Understanding when()"
    and included an example of conditional branches in pseudocode early on
    and a runnable example later.
  • I also added an additional subheading for "Linking across charts"
  • Included mention of the use of empty=False inside when together
    with a short pseudocode example.
  • Creating a new section at the end about combining parameters,
    which contains both the previous section on parameter composition,
    but also two new subsection on multiple conditions and conditional
    when branches.
  • I expanded on the parameter composition examples as well.

Please have a look and let me know what you think
I moved the PR out of draft and will go through your other examples today or tomorrow.
I noticed a few unrelated doc changes that I will include in a follow up PR after we merge this.

Comment on lines +603 to +606
# empty=False ensure that no points are selected before a selection is drawn
brush = alt.selection_interval(empty=False)
brush2 = alt.selection_interval(empty=False)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this and the next example will not quite work as intended until #3598 is fixed, but I don't think that is blocking for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this and the next example will not quite work as intended until #3598 is fixed, but I don't think that is blocking for this PR.

I tried this, but it seems to break bars

verbose_compose = alt.LogicalAndPredicate(
    **{"and": [dict(brush.to_dict(), empty=False), dict(brush2.to_dict(), empty=False)]}
)

After seeing

if isinstance(filter, Parameter):
new_filter: dict[str, Any] = {"param": filter.name}
if "empty" in kwargs:
new_filter["empty"] = kwargs.pop("empty")
elif isinstance(filter.empty, bool):
new_filter["empty"] = filter.empty
filter = new_filter
return self._add_transform(core.FilterTransform(filter=filter, **kwargs))

I also tried this:

bars = base.mark_bar().encode(x="count()", y="Origin:N").transform_filter(brush & brush2, empty=False)
Output

SchemaValidationError: `FilterTransform` has no parameter named 'empty'

Existing parameter names are:
filter   

See the help for `FilterTransform` to read the full description of these parameters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a cool example, but if it isn't working as you'd like - maybe its best to add in a follow up PR?

Since it doesn't use alt.when, I don't think we need to rush it into this PR

@joelostblom joelostblom marked this pull request as ready for review September 19, 2024 19:24
Copy link
Member Author

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking the reorganization @joelostblom!

I picked up a few rst corrections that need to be made, and some questions/suggestions on content.

Overall I think we're very close, really appreciate your help with this PR 🤝


Multiple parameters can be combined in a single chart,
either via multiple separate response conditions,
different conditional branches in :fun:`when`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
different conditional branches in :fun:`when`,
different conditional branches in :func:`when`,

Conditional branches
^^^^^^^^^^^^^^^^^^^^

:fun:`when` allows the use of multiple ``then`` (``elif``) branches
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:fun:`when` allows the use of multiple ``then`` (``elif``) branches
:func:`when` allows the use of multiple ``then`` (``elif``) branches

)

either_or_but_not_both = (alex | morgan) & ~(alex & morgan)
Copy link
Member Author

@dangotbanned dangotbanned Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be clearer using exclusive or?

I've noticed we don't have an __xor__ (^) defined for alt.expr.core.OperatorMixin, but we do have implementations for most others.

js bitwise operators include XOR and use the same symbol (^).


  1. Rename variable to align closer to exclusive or. You can always link to another resource (probably not wiki) to explain the concept in more detail. Or show a truth table?
  2. Look into adding alt.expr.core.OperatorMixin.__xor__ in a new issue (non-blocking here)

Comment on lines +204 to +205
As a convenience,
simple conditions may be expressed without defining a default:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to add to the original quote:

Simple conditions may be expressed without defining a default

I'm not sure "As a convenience" is the right fit.
It isn't really a convenience of the syntax itself, but functionality inherited from vega-lite - which wasn't possible using alt.condition since it required if_false.

I would think commenting on what value would be used instead may be more helpful.
Specifically, it would fall back to the default value of the given channel.

From the end of vega-lite condition

The next plot uses a conditional value definition to color data points with null values in grey. Note that if the “else” case value is not specified, default mark color will be applied.

I adapted that here:

Omitting the ``.otherwise()`` clause will use the channel default instead::
points.encode(color=alt.when(brush).then("Origin"))

)
points

Multiple conditional branches (``elif`` in Python)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick with the same style used in polars.when for this.

Chained when-then operations should be read as Python if, elif, ... elif blocks,

One of these maybe?

1

Multiple conditional branches (``if, elif, ..., elif`` in Python)

2

Multiple conditional branches (``if-elif-else`` in Python)

3

Multiple conditional branches (``if-elif-...-else`` in Python)

Comment on lines +303 to +305


.. altair-plot::
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.. altair-plot::

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was causing an empty code block:

image

The ``empty=False`` argument could instead be set inside :func:`when`,
so that you don't need to redefine the same selection, e.g.::

brush = alt.selection()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
brush = alt.selection()
brush = alt.selection_interval()

Comment on lines +402 to +403
The ``empty=False`` argument could instead be set inside :func:`when`,
so that you don't need to redefine the same selection, e.g.::
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a more detailed comment here but it seems to not have saved 😞

The motivation for using empty=False outside of the parameter definition is to change the behavior for the condition, not the parameter.

brush will still have empty=True on every other use

empty
For selection parameters, the predicate of empty selections returns ``True`` by default.
Override this behavior, with ``empty=False``.
.. note::
When ``predicate`` is a ``Parameter`` that is used more than once,
``alt.when(..., empty=...)`` provides granular control for each occurrence.

Related

Comment on lines +603 to +606
# empty=False ensure that no points are selected before a selection is drawn
brush = alt.selection_interval(empty=False)
brush2 = alt.selection_interval(empty=False)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this and the next example will not quite work as intended until #3598 is fixed, but I don't think that is blocking for this PR.

I tried this, but it seems to break bars

verbose_compose = alt.LogicalAndPredicate(
    **{"and": [dict(brush.to_dict(), empty=False), dict(brush2.to_dict(), empty=False)]}
)

After seeing

if isinstance(filter, Parameter):
new_filter: dict[str, Any] = {"param": filter.name}
if "empty" in kwargs:
new_filter["empty"] = kwargs.pop("empty")
elif isinstance(filter.empty, bool):
new_filter["empty"] = filter.empty
filter = new_filter
return self._add_transform(core.FilterTransform(filter=filter, **kwargs))

I also tried this:

bars = base.mark_bar().encode(x="count()", y="Origin:N").transform_filter(brush & brush2, empty=False)
Output

SchemaValidationError: `FilterTransform` has no parameter named 'empty'

Existing parameter names are:
filter   

See the help for `FilterTransform` to read the full description of these parameters

Comment on lines +603 to +606
# empty=False ensure that no points are selected before a selection is drawn
brush = alt.selection_interval(empty=False)
brush2 = alt.selection_interval(empty=False)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a cool example, but if it isn't working as you'd like - maybe its best to add in a follow up PR?

Since it doesn't use alt.when, I don't think we need to rush it into this PR

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

Successfully merging this pull request may close these issues.

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