Repository: altair-viz/altair
Pull request #2841 (2023-01-20 00:09:35)

Parse shorthand when creating the condition 2
This is a cleaned up version of #2837 

Also updated one test to the new condition handling.

ChristopherDavisUCI: Two of the failed tests `test_selection_to_dict` I think are specific to Vega-Lite v3 and v4 (not v5).  (I made a change to the v5 api but not to v3 or v4.)  Does anyone see anything specific about the other failed tests?
binste: @ChristopherDavisUCI Thanks for looking into this! The work you and @joelostblom put into this got me thinking about the whole deep validation approach in general and if there is not a better way using more of the information that `jsonschema` returns when validating the whole schema, see https://github.com/altair-viz/altair/pull/2835#issuecomment-1397537047. I'll open a PR soon which explains it in detail but wanted to give you a heads-up that I got it working and it solves this issue and many others we have around error messages.
joelostblom: Closed in favor of #2842 
ChristopherDavisUCI: @joelostblom I think some of the code here could still be worth considering.  In the current master branch, the code
```
c = alt.Chart().mark_point().encode(
    size = alt.condition('true', alt.value(100), alt.value(10))
)
```
will result in `c.encoding.size` being equal to
```
SizeValue({
  condition: SizeValue({
    test: 'true',
    value: 100
  }),
  value: 10
})
```
That inner `SizeValue` does not seem correct (because it wraps something which is not a valid `SizeValue`).

Also, if we evaluate `c.to_json(validate="deep")` on the above chart, an error is still raised, even though I think the chart itself is fine.

Would it be worth me opening a new PR with some of this same code?  Or do you see any reason this inner wrapping is desirable?

@binste I'm also happy to hear your thoughts!
joelostblom: > That inner SizeValue does not seem correct (because it wraps something which is not a valid SizeValue).

I agree with you that this looks odd, and the change you suggested looks more appropriate (although both `to_json` and `to_dict` still works fine). I tracked the introduction of the behavior you removed to this PR https://github.com/altair-viz/altair/pull/1597. Could you investigate that closer and see if there was any specific intention with introducing that double condition wrapping that we risk losing here?

I will reopen this for now so that we can continue the discussion and see the tests passing. Could you rebase it on the current master branch?

> Also, if we evaluate c.to_json(validate="deep") on the above chart, an error is still raised, even though I think the chart itself is fine.

Note that this no longer happens after #2842 since we removed the `deep` parameter.

ChristopherDavisUCI: Thanks @joelostblom!

I think `c.to_json(validate="deep")` still exists even after #2842 (maybe it just doesn't get called automatically).

I will investigate a little more the origin of this wrapping.  I think it first showed up in #646, but that seems like a pretty major PR, so it's a little hard for me to grasp at first glance.

Do you know, when I see an error in the automated tests like the following, is there a good way to tell which of the example files caused this error to be raised?  Neither on GitHub nor locally with VS Code is it clear to me how to tell which example file was being used.  (I tried running the test with debug mode in VS Code, but so far I have not gotten it to halt when the error is raised.)
<img width="1013" alt="Screenshot 2023-02-10 at 8 24 50 AM" src="https://user-images.githubusercontent.com/82750240/218144770-0798f67a-4078-4251-9013-ed64c69b350d.png">

It seems like code somewhere is expecting `copy.condition` to be a dictionary, where it is actually a `_PropertySetter`.
mattijn: Maybe add temporarily `print(filename)` around here while running the test in debug modus in VSCode?:
```python
def test_render_examples_to_chart(syntax_module):
    for filename in iter_examples_filenames(syntax_module):
        print(filename)
```
The `test_render_examples_to_chart ` function is defined using a `@pytest.mark.parametrize(...)`, you can click right-mouse click on the green debug triangles and select which test within the `parameterize` you like to run in a debug session:
<img width="545" alt="image" src="https://user-images.githubusercontent.com/5186265/218276922-1f9d2aba-6504-44d2-90d2-b105419f5a71.png">

ChristopherDavisUCI: Thanks @mattijn!  I haven't had a chance to experiment yet, but I should have time this week.
ChristopherDavisUCI: @joelostblom @mattijn I believe this is ready for review.

I found the source of the tests failing before... it was a `copy.condition` which needed to be changed to `copy["condition"]` because of the new property setters.

The main sales pitch for this PR is what was described in https://github.com/altair-viz/altair/pull/2841#issuecomment-1422639767.  Briefly, I think if `c.to_json()` is a valid spec, then `c.to_json(validate="deep")` should not raise an error.

The main change in this PR is when an `if_true` condition like `"Horsepower:Q"` gets expanded from that shorthand notation.  This PR makes this expansion occur earlier.  I believe the functionality that's being replaced was introduced in #646.  That PR is about shorthand parsing in general, not specifically related to conditions.

Patch:
diff --git a/altair/utils/__init__.py b/altair/utils/__init__.py
index cb7920b0..0bd8ec5e 100644
--- a/altair/utils/__init__.py
+++ b/altair/utils/__init__.py
@@ -7,11 +7,11 @@ from .core import (
     update_nested,
     display_traceback,
     SchemaBase,
-    Undefined,
 )
 from .html import spec_to_html
 from .plugin_registry import PluginRegistry
 from .deprecation import AltairDeprecationWarning
+from .schemapi import Undefined
 
 
 __all__ = (
diff --git a/altair/utils/core.py b/altair/utils/core.py
index c47b9a04..38bf0950 100644
--- a/altair/utils/core.py
+++ b/altair/utils/core.py
@@ -14,7 +14,7 @@ import jsonschema
 import pandas as pd
 import numpy as np
 
-from altair.utils.schemapi import SchemaBase, Undefined
+from altair.utils.schemapi import SchemaBase
 
 try:
     from pandas.api.types import infer_dtype as _infer_dtype
@@ -669,15 +669,6 @@ def infer_encoding_types(args, kwargs, channels):
         kwargs[encoding] = arg
 
     def _wrap_in_channel_class(obj, encoding):
-        try:
-            condition = obj["condition"]
-        except (KeyError, TypeError):
-            pass
-        else:
-            if condition is not Undefined:
-                obj = obj.copy()
-                obj["condition"] = _wrap_in_channel_class(condition, encoding)
-
         if isinstance(obj, SchemaBase):
             return obj
 
diff --git a/altair/vegalite/v5/api.py b/altair/vegalite/v5/api.py
index 478c3e5d..5a65f86b 100644
--- a/altair/vegalite/v5/api.py
+++ b/altair/vegalite/v5/api.py
@@ -516,7 +516,7 @@ def condition(predicate, if_true, if_false, **kwargs):
         # dict in the appropriate schema
         if_true = if_true.to_dict()
     elif isinstance(if_true, str):
-        if_true = {"shorthand": if_true}
+        if_true = utils.parse_shorthand(if_true)
         if_true.update(kwargs)
     condition.update(if_true)
 
diff --git a/altair/vegalite/v5/schema/channels.py b/altair/vegalite/v5/schema/channels.py
index bb078f59..a59f4bd1 100644
--- a/altair/vegalite/v5/schema/channels.py
+++ b/altair/vegalite/v5/schema/channels.py
@@ -70,7 +70,7 @@ class ValueChannelMixin(object):
             elif 'field' in condition and 'type' not in condition:
                 kwds = parse_shorthand(condition['field'], context.get('data', None))
                 copy = self.copy(deep=['condition'])
-                copy.condition.update(kwds)
+                copy['condition'].update(kwds)
         return super(ValueChannelMixin, copy).to_dict(validate=validate,
                                                       ignore=ignore,
                                                       context=context)


Test patch:
diff --git a/tests/utils/tests/test_core.py b/tests/utils/tests/test_core.py
index c46d1744..e0b57567 100644
--- a/tests/utils/tests/test_core.py
+++ b/tests/utils/tests/test_core.py
@@ -249,17 +249,35 @@ def test_infer_encoding_types(channels):
     assert infer_encoding_types(args, kwds, channels) == expected
 
 
-def test_infer_encoding_types_with_condition(channels):
+def test_infer_encoding_types_with_condition():
+    channels = alt.channels
+
     args, kwds = _getargs(
-        x=alt.condition("pred1", alt.value(1), alt.value(2)),
-        y=alt.condition("pred2", alt.value(1), "yval"),
-        strokeWidth=alt.condition("pred3", "sval", alt.value(2)),
+        size=alt.condition("pred1", alt.value(1), alt.value(2)),
+        color=alt.condition("pred2", alt.value("red"), "cfield:N"),
+        opacity=alt.condition("pred3", "ofield:N", alt.value(0.2)),
     )
+
     expected = dict(
-        x=channels.XValue(2, condition=channels.XValue(1, test="pred1")),
-        y=channels.Y("yval", condition=channels.YValue(1, test="pred2")),
-        strokeWidth=channels.StrokeWidthValue(
-            2, condition=channels.StrokeWidth("sval", test="pred3")
+        size=channels.SizeValue(
+            2,
+            condition=alt.ConditionalPredicateValueDefnumberExprRef(
+                value=1, test=alt.Predicate("pred1")
+            ),
+        ),
+        color=channels.Color(
+            "cfield:N",
+            condition=alt.ConditionalPredicateValueDefGradientstringnullExprRef(
+                value="red", test=alt.Predicate("pred2")
+            ),
+        ),
+        opacity=channels.OpacityValue(
+            0.2,
+            condition=alt.ConditionalPredicateMarkPropFieldOrDatumDef(
+                field=alt.FieldName("ofield"),
+                test=alt.Predicate("pred3"),
+                type=alt.StandardType("nominal"),
+            ),
         ),
     )
     assert infer_encoding_types(args, kwds, channels) == expected