Map ordinal selected values against their position in the domain#1379
Map ordinal selected values against their position in the domain#1379martinRenou merged 4 commits intobqplot:masterfrom
Conversation
|
|
||
| typedRange(values) { | ||
| return new Int32Array(values.map(Number)); | ||
| return new Int32Array(values.map((value) => this.domain.indexOf(value))); |
There was a problem hiding this comment.
could it just be:
return new Int32Array(values.map(this.domain.indexOf));this would avoid allocating a new function/scope each time... though maybe the this shenanigans come into play...
There was a problem hiding this comment.
I originally tried that, but it was throwing errors... probably due to this shenanigans.
There was a problem hiding this comment.
k, how about:
return new Int32Array(values.map(this.domain.indexOf, this.domain));There was a problem hiding this comment.
Yes that behaves and pushed the change.
|
W.r.t. the test, the closest PR I can find didn't add a test, and looking through the existing tests, I'm not sure how we would add a test for this. Maybe @martinRenou or @maartenbreddels can you point us to a PR that did something similar and had a test for it? The process for producing the error is here, but it requires manual brushing. |
|
Since recently we added galata tests in bqplot. Galata should have everything for "manually" testing the brush and checking that it renders properly. But that requires good knowledge of galata which I don't have... Can this be bug be triggered programmatically from Python? That should help create a visual test. I can look into it. |
|
Thanks @martinRenou, it looks like that should work. However, in investigating programmatically setting the brush This PR originally introduces using the domain index for the values in the brush's typedRange(values) {
return new Int32Array(values.map(this.domain.indexOf, this.domain));
}could be replaced with: typedRange(values) {
return values;
}There is also a new galata test notebook |
@martinRenou : @dfreeman06 added a test using Galata that we believe is working and testing the brushes programmatically, at least locally it is passing, would it be possible to run the workflow to see if everything is on the up and up? |
|
Just a friendly ping to the BQPlot team to see if there is something else we should do for this PR. Also curious if @maartenbreddels has any issues with this as per his PR. |
|
|
||
| typedRange(values) { | ||
| return new Int32Array(values.map(Number)); | ||
| return values; |
There was a problem hiding this comment.
As per @bollwyvl 's recommendation, this could be:
return new Int32Array(values.map(this.domain.indexOf, this.domain));Which would make the scale work with the indeces of the domain (I believe that is what is meant by range here, @maartenbreddels please don't hesitate to let us know if we misunderstood what was intended by range).
We went with the values instead because otherwise it could break other people's code, but we acknowledge it would probably be better to use the line above.
|
y'all let me know if there's anything i can do to push this forward... |
sanbales
left a comment
There was a problem hiding this comment.
Not sure non-contributor reviews matter, but in case it does, here is my approval.
|
Thanks for adding tests. Ideally, we would add a galata test that simulates a selection with the mouse and test that the Thanks! |
|
Everybody's real busy, for sure, but Is there a timeline for a |
Attempt to address #1378. Mapping the Ordinal selected values from a brush through their index position in the domain.