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

feat: move selection emptiness check to predicate #7155

Merged
merged 3 commits into from
Jan 15, 2021

Conversation

arvind
Copy link
Member

@arvind arvind commented Jan 5, 2021

Addresses #4371 — emptiness checks on the predicate allow us to get rid of the hacky length(data("brush_store")) hacks of yore. This PR, however, moves the emptiness check to the selection predicate rather than allowing emptiness to be tested with the predicate in addition to being defined on the selection as before (#3038).

Although this approach can introduce some occasional duplication, I think it's preferable to have only one way of doing things and because keeping the selection-level emptiness definition would yield a somewhat confusing set of "empty" property values ("all" | "none" at the selection level, and then booleans at the predicate level).

Adding empty to the predicate level necessitates one other breaking change: selection predicates can only be composed alongside other predicate types (i.e., within a condition.test block or filter) rather than being composable within the selection property itself (i.e., we could previously do "selection": {"and": ["foo", "bar"]}).

@arvind arvind force-pushed the as/empty-predicate branch 2 times, most recently from c5c2fc8 to 28d6d8a Compare January 5, 2021 23:48
@arvind arvind marked this pull request as ready for review January 5, 2021 23:58
@arvind arvind requested a review from a team January 5, 2021 23:58
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Looks good except for one issue with an example.

Does it fix #4371?

@@ -298,7 +298,7 @@
"update": {
"fill": [
{
"test": "!(length(data(\"pts_store\"))) || (!(vlSelectionTest(\"pts_store\", datum)))",
"test": "!(!length(data(\"pts_store\")) || vlSelectionTest(\"pts_store\", datum))",
Copy link
Member

Choose a reason for hiding this comment

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

This example is now different from before. Is that intended?

The docs say "Notice, all marks are initially colored grey. This is because empty selections are treated as containing all data values." so I think either the example or the docs need to change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, good catch! I was concerned about this as well given what the docs said. But, as far as I can tell, this was a bug in the previous way we were constructing selection predicates and the docs are incorrectly and I fixed it in the previous commit: 28d6d8a#diff-a4cbaf0ac9270d18fd49ffa34980748d840aa87e9983abdf94b9f1088edea0ca. Explanation below.

From the docs:

a conditional value definition sets the rect marks to grey when they do not lie within the selection, and a regular field mapping is used otherwise.

So, in both our prior and this PR's approach, by default, when a selection is empty all data values are considered to lie within the selection. Thus, based on the above description of the spec, the initial state of the visualization begins with an empty selection, all values thus lie within the selection, and the "selection": "pts" predicate should return true. This is then inverted "not": {"selection": "pts"} and should become false for all data values, and the marks should be assigned else branch of the condition.

The sort-of double negation definitely confused me for a bit (and is likely why the docs were written the way they were to start with) so please double check my work and make sure I'm not making a mistake :)

@domoritz
Copy link
Member

domoritz commented Jan 6, 2021

@kanitw since you filed #4371, can you take a look?

@arvind
Copy link
Member Author

arvind commented Jan 6, 2021

Yes, fixes #4371.

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

Successfully merging this pull request may close these issues.

2 participants