Refactor + Standardize Selection State Representation #4068
Conversation
|
Ok, updated and verified that the runtime tests are passing locally*. They won't pass Travis until the new expression functions land in Vega. So, this PR is now ready for review. * = I've disabled runtime tests for selections resolved to |
|
Siiiigh, the issue with |
|
I added a blocked tag. We can remove it once the necessary changes land in Vega. |
|
I guess we are waiting for a Vega release before we merge this. Is there anything else left? |
|
Yes, I think so. |
|
Not sure why, buy a bunch of SVGs are gone in this PR. |
|
@arvind do you know what's up? |
|
Huh, that's a great question. Looks like all of those deletions seem to have happened via the |
|
One thought: is it actually deletions or an error in generating the files? It looks like they're all 1 byte sized, and viewing them in the rich diff view produces an "file is invalid" error. So perhaps Travis is having difficulty producing the SVGs for some reason? |
|
One idea: previous runs of the CI did not have the necessary Vega version and thus parsing the specs would produce an error. Perhaps we're not resilient to that and overwrite output SVGs with null/empty output? Going to validate this hypothesis by force pushing and forcing Travis to rebuild the examples now that the correct Vega version is available. |
|
Wow, I'm really confused about what is going on. After the runtime tests execute, Travis seems to go completely off the rails and no longer gets to the example generation stage. Hmm. |
This signal centralizes definition of whether selection tuples enumerate values or define ranges, and is the first step to unifying tuple structures across selection types.
Removing information now encapsulated by the tuple definition signal (8203361).
Interval selections define inclusive ranges. Discrete bin selections, however, should define a right-exclusive range, i.e., in the range [bin_start, bin_end).
A standard tuple structure for all selection types means we no longer need the separate predicate test functions.
A top-level signal does not account for different views having different field/channel/type bindings for the same selection.
|
@domoritz Looks like the error with SVGs has gone away with the fixes to the Travis process. So this PR should now be actually ready for review. |
|
Thanks for the LGTM and the code cleanup @domoritz. I squashed #4216 into this PR but do you feel strongly about squashing this PR into master? My preference would be to rebase it to preserve the granularity of code commits in what is a pretty major refactoring. It'll help (me, in particular) remember design decisions more easily. I worry that squashing it will produce a single, extremely complex commit and it will be hard to untangle all the various components that went into the refactoring. (I'm also working on improving the test coverage of |
|
The advantage of squashing is that we can read the commits to write the changelog. Moreover, you get a reference to the PR in the commit message. If we need to look into what went into a commit, we can go to the PR, read the comments and also see the individual commits. This only works if you go to Github. We have squashed complex PRs in the past. For this PR, I'm also okay with merging if you use |
With scale bindings, the top-level signal needs to be assembled from the constituent top-level channelSignals which contain the correct state of the selection (i.e., the unit selection most recently interacted with). This commit also broadens test coverage of selection-scale bindings.
… initialize fill array (#4216) * Replace forEach with for, remove as, add tests and initialize fill array * Get rid of push.apply * Unused import * Replace more forEach with for..of.
|
Thanks for describing the rationale. The alternate workflow you propose makes sense to me, so I'll go ahead and squash this PR. |
**This PR relies on the refactoring introduced by #4068, so please merge that one first.** This PR adds an `init` property to selection definitions to initialize them using the fields or encoding channels a selection is projected over. For example, here is an updated version of our `interactive_query_widgets` example that initializes the selection to select cars with 8 cylinders manufactured in 1971. ```json { "$schema": "https://vega.github.io/schema/vega-lite/v2.json", "description": "Drag the sliders to highlight points.", "data": {"url": "data/cars.json"}, "transform": [{"calculate": "year(datum.Year)", "as": "Year"}], "layer": [{ "selection": { "CylYr": { "type": "single", "fields": ["Cylinders", "Year"], "init": {"Cylinders": 4, "Year": 1977}, "bind": { "Cylinders": {"input": "range", "min": 3, "max": 8, "step": 1}, "Year": {"input": "range", "min": 1969, "max": 1981, "step": 1} } } }, "mark": "circle", "encoding": { "x": {"field": "Horsepower", "type": "quantitative"}, "y": {"field": "Miles_per_Gallon", "type": "quantitative"}, "color": { "condition": {"selection": "CylYr", "field": "Origin", "type": "nominal"}, "value": "grey" } } }, { "transform": [{"filter": {"selection": "CylYr"}}], "mark": "circle", "encoding": { "x": {"field": "Horsepower", "type": "quantitative"}, "y": {"field": "Miles_per_Gallon", "type": "quantitative"}, "color": {"field": "Origin", "type": "nominal"}, "size": {"value": 100} } }] } ``` Similarly, an updated `interactive_brush` example to initialize the brush selection is as follows: ```json { "$schema": "https://vega.github.io/schema/vega-lite/v2.json", "description": "Drag out a rectangular brush to highlight points.", "data": {"url": "data/cars.json"}, "selection": { "brush": { "type": "interval", "init": {"x": [55, 160], "y": [13, 37]} } }, "mark": "point", "encoding": { "x": {"field": "Horsepower", "type": "quantitative"}, "y": {"field": "Miles_per_Gallon", "type": "quantitative"}, "color": { "condition": {"selection": "brush", "field": "Cylinders", "type": "ordinal"}, "value": "grey" } } } ``` Fixes #3409, #3431, and vega/altair#599. /cc @jakevdp
A major refactoring of how a selection state is stored within its dataset. This state uses tuples of the form:
where
fielddefis an object of the form:A
fielddef'stypeidentifies whether the values being stored are simple enumerations (E) or represent ranges (R) of different kinds (e.g., a right-exclusive range would beR-RE).This new state requires corresponding new Vega expression functions (vega/vega-parser#89). Besides simplifying several code paths, it's also allowed us to much more easily add a top-level signal named (partially addressing #1830 with a much simpler strategy than I described here: #1830 (comment)).
I've marked this PR as WIP until I've had a chance to test the changes more thoroughly, but it's ready for high-level feedback.
Note: While this PR does not introduce any breaking changes to the Vega-Lite language, it is tied to whatever Vega version vega/vega-parser#89 lands in. So, in that sense, it is a "breaking change" that might warrant us bumping the major version number of Vega-Lite.