feat: Lasso Specification Compiler #8104
Conversation
There was a problem hiding this comment.
Hi @dvmoritzschoefl, I'm so sorry for being so delayed on reviewing this PR. I've left you some minor comments throughout. I think there are two larger things pending, one of which you can likely address independently and the other you're currently blocking on me:
- Compile-time tests: these check to ensure that the correct output Vega JSON specs are produced for the selection type and its various constituent parameters. Take a look at the existing compile-time tests for selections for some examples.
- Run-time tests: we discussed these on Slack. I'll aim to have some instructions for you about this by Friday.
Thanks again for your hard work implementing this feature, and for bearing with my slow response over the past few months. I think lasso selections are going to be enormously beneficial for and well-received by our users!
|
@arvind I added runtime and compile tests for the lasso selection. I also refactored lasso to region in the comments and classes. |
There was a problem hiding this comment.
Thanks @dvmoritzschoefl! This looks good to me, and a very exciting feature for our users!!
src/selection.ts
Outdated
| * | ||
| * __See also:__ The [projection with `encodings` and `fields` section](https://vega.github.io/vega-lite/docs/selection.html#project) in the documentation. | ||
| */ | ||
| fields?: FieldName[]; |
There was a problem hiding this comment.
How should this property work? Shouldn't the fields always match the x and y encodings?
It would be good to add an example for this property if it should be supported, or otherwise we should remove them.
There was a problem hiding this comment.
Hi, looking at the vega counterpart for this PR
https://github.com/vega/vega/pull/3388/files#diff-547caeeef0ceae19c314c72edcd93ab9aac4a9462ada8fbae25ec7d37fad147b
the intersection test will always use x and y coordinates, if this is what you meant.
I think I took the interval selection config as a starting point, can I just remove the fields property if it is not needed?
EDIT:
Shouldn´t the fields property be always [SELECTION_ID] like in the default config for the point selection? The interval selection uses x/y channel bounds or ids as values, but for the region only ids are possible (like in the point case). And looking at line 83 the point selection also has a fields property.
There was a problem hiding this comment.
Shouldn´t the fields property be always [SELECTION_ID] like in the default config for the point selection?
Point selection indeed uses [SELECTION_ID] by default. However, one can configure the fields to be different for point selection, thus having the fields property for users to customize makes sense.
So the question is whether customizing this for lasso ever make sense.
If it should always be a constant value and can't be customized, we shouldn't expose it in the specification.
If it can be customized, better add an example to demonstrate at least one practical use case.
There was a problem hiding this comment.
@dvmoritzschoefl: the typings here generate the JSON Schema and/or serve as the public interface for what Vega-Lite users believe can be specified. You're quite right that the lasso selection's predicate uses SELECTION_ID, but it does not make sense to customize it to use other fields (i.e., your predicate function is hard coded to vlSelectionIdTest) or for customizing the field to affect how lassoing works operationally/mechanically (i.e., how we can get a uni-dimensional brush by specifying "encodings": ["x"] for interval selections).
So I think @kanitw is quite right that this property should be dropped here — great catch @kanitw!
There was a problem hiding this comment.
Ok, thanks for the clarification that makes sense. So removing both this property and the initial config for it should do the trick?
There was a problem hiding this comment.
@kanitw It seems that the CLI is generating the faulty images after a new commit. Is there an easy way to test why it is generating empty images? I tested the examples locally and they seem correct. Also the other images are correctly created in the runtime tests.
This seems to be the incorrect location anyway for the images. I think I generated them in the wrong place once and did not notice, the only place where those are generated is by running the runtime tests right? (Then I can just delete them and it run it again) |
|
Closing this in favor of #8988 which freshens the code, expands runtime test coverage, and fixes a few bugs along the way. THANK YOU @dvmoritzschoefl for all your contributions here, and I'm very sorry it's taken us such a long time to actually make progress on getting it merged. |
This is the vega-lite implementation for lasso selections. It is compatible with the current vega master branch.