Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Lasso Specification Compiler #8104
feat: Lasso Specification Compiler #8104
Changes from 3 commits
c4dce86
ef30d64
2b265ec
a69d095
14c8ae0
f9280c7
80a46c4
29e9de2
2e713d1
57cf61b
20a2b0c
f961399
4c42674
e5c56b9
caacbec
e9df727
de0a0d5
07e7164
537c09a
2a5d2f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point selection indeed uses
[SELECTION_ID]
by default. However, one can configure thefields
to be different for point selection, thus having thefields
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 tovlSelectionIdTest
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kanitw @arvind Was I correct under the assumption that the 2 images were in the wrong folder? Is it resolved then or is there another thing to do before this can be merged?