feat: add field predicate types to selectionTest in vega-selections#3675
feat: add field predicate types to selectionTest in vega-selections#3675
Conversation
|
i believe this supersedes #1839 which is also very stale anyway |
| if (!inrange(dval, values[i], false, false)) return false; | ||
| } else if (f.type === TYPE_RANGE_LE) { | ||
| if (!inrange(dval, values[i], false, true)) return false; | ||
| } else if (f.type === TYPE_PRED_LT) { |
There was a problem hiding this comment.
Should this become a switch now?
There was a problem hiding this comment.
i personally don't find switch to be clearer in this instance because all of the returns are conditional, so we'd have to add break statements to every one and remember to keep adding them if we change it in the future. IMO, not shorter, possibly less maintainable, not a significant change in readability. however, happy to make the change if you feel otherwise
There was a problem hiding this comment.
Makes sense. I don't have a strong opinion. You could pull the logic here out into a function and then use a return in each branch.
There was a problem hiding this comment.
the if statements only return false because it's an early exit from the loop (none of the comparisons can be false for the test to return true), so we can't use an unconditional return inside the switch without changing the loop too
presumably it is written this way for efficiency(tm) over readability? but it did take me trying to rewrite it to realize this so maybe it should just be made more readable?
what do you think of e.g.
fields.every(f => {
switch (f.type) {
case TYPE_ENUM:
return ...;
case ...
}
})
There was a problem hiding this comment.
Your intuition is correct @jonathanzong! Because this code path is typically called against every visualized data point on every interaction (eg potentially every mouse drag), we need it to be really performant. When this was first written, most browsers were still faster at evaluating an old school for loop. It was also one of the reasons we minimized functions calls (because they can—or at least could—incur performance overhead).
So, while I’m sympathetic to readability concerns, if we really wanted to make a change here (including extracting this to another function call), perhaps we could run a micro benchmark to validate whether it would cause a performance hit?
(My guess is browsers have advanced significantly since these code paths were first written, so I wouldn’t be surprised if some of these reasons no longer hold. On the other hand, back in the day, I do remember being surprised by what syntaxes did and didn’t incur performance penalties!)
|
bump @arvind |
|
@arvind can you wrap up the review and merge if it looks good? |
|
I found a potential problem here: ops in I'm pretty confused but can look into this later but let's not merge this yet until then. |
|
I converted this pull request to a draft. Please mark it as ready. |
|
this is ready for review now |
arvind
left a comment
There was a problem hiding this comment.
Hi @jonathanzong, so very sorry for lagging behind on approving this (to put it mildly). The additional predicate types and expansion of the if-block look good to me. But, I'm not sure whether the E prefix is what we want.
To clarify, the selectionResolve code path gets called when a selection is defined within a faceted or repeated view (i.e., there is an instance of the selection for each facet/repeated unit and we need a way of resolving them to produce an overall predicate that is applied to the other views).
The type here is not about the predicate comparator but rather the terms of the predicate (what the Vega-Lite codebase names TupleStoreType). Essentially, are the predicates enumerated values (typically generated by point selections or brushes over cartographic projections) or express a two tuple range (typically generated by brushes or point selections over binned scales).
So, I'm not sure that using the E (for enum) prefix across all the new predicate types is what we want? I think it makes sense for VALID and ONE_OF since I expect those probably operate a list of discrete values. But, for the inequalities, I would imagine they should be R?
Relatedly, have we implemented tests somewhere in the stack to validate these functions? (Thus far, I think these functions have been tested in the Vega-Lite codebase.) I just want to make sure since I'd be nervous if parts of this PR are designed speculatively.
|
I believe that Example: "a is between 55 and 160" vs "a is greater than 100" I think this is consistent with the point/interval metaphor because you could imagine clicking a point and intending to select e.g. "everything to the right of this point on the x axis". So far, I've been testing these in Umwelt and Animated Vega-Lite at runtime. I don't think I'm at a point where I could add runtime tests to Vega-Lite for the new predicate types, since there isn't currently anything in Vega-Lite that generates these tuples. But hopefully the unit tests help us confirm this functionality works as intended and helps us move confidently once we get to the point where we're enabling more types of predicates in VL (e.g. in a future PR for Animated VL or my dream which is to rework the selection spec). |
|
Aah this makes sense to me. Thanks for explaining it and for adding |
changes since v5.30.0 **vega-utils** * use `Object.hasOwn` instead of `Object.prototype.hasOwnProperty` (via #3951). (Thanks @domoritz!) **vega-parser** * Add discrete legend type (via #3957). (Thanks @hydrosquall!) **vega-functions** * Add sort function to vega-functions (and vega-interpreter) (via #3973). (Thanks @hydrosquall!) **vega-selections** * Add field predicate types to selectionTest (via #3675). (Thanks @jonathanzong!) **monorepo** * Replace flights-5k.json external reference (via #3999). (Thanks @dwootton!) **docs** * Update packed bubble example (via #3955). (Thanks @PBI-David!) * Correct typo in production rules documentation (via #3958). (Thanks @shanebruggeman!) * Update README.md to fix broken link to current roadmap (via #3979). (Thanks @cahogan & @joelostblom!) --------- Signed-off-by: Lukas Hermann <1734032+lsh@users.noreply.github.com>
This change introduces the ability for
vlSelectionTestto check for more types of field predicates.This additive change will have no immediate effect, since Vega-Lite doesn't currently produce selection tuples that use these predicate types. However, future proposed changes for Animated Vega-Lite will require us to support these field predicates in addition to the existing point and interval predicates.
cc @arvind