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

bisector no longer supports two-argument (object, value) comparator #249

Closed
louking opened this issue Apr 8, 2022 · 12 comments · Fixed by #250
Closed

bisector no longer supports two-argument (object, value) comparator #249

louking opened this issue Apr 8, 2022 · 12 comments · Fixed by #250

Comments

@louking
Copy link

louking commented Apr 8, 2022

It looks like 22cdb3f broke my compare function, which used an array of objects.

I see I can use array of values, but wanted to point out this breaking change.

My compare function was

            that.bisectX = d3.bisector(function (d, x) {
                if (that.xascending) {
                    return d.x - x;
                } else {
                    return x - d.x;
                }
            }).left;

The breaking change is at

if (compare1(x, x) !== 0) return hi;

@mbostock
Copy link
Member

mbostock commented Apr 8, 2022

Thanks for the feedback. This was changed in 3.0.2 to better handle undefined order. We consider this a bug fix. You can read about why we made this changes in #217 #219 #227.

@mbostock mbostock closed this as completed Apr 8, 2022
@louking
Copy link
Author

louking commented Apr 8, 2022

So you're saying bisector() compare function no longer supports object, right? Even though this case is mentioned (for sort()) in #217 (comment)

@mbostock
Copy link
Member

mbostock commented Apr 8, 2022

I don’t follow. What are you trying to do?

@louking
Copy link
Author

louking commented Apr 8, 2022

briefly: I have array of objects d, and want to bisect by d.x

the objects happen to have the x,y values

I can create new array of all d.x values if needed, but the change broke my current compare function

@mbostock
Copy link
Member

mbostock commented Apr 8, 2022

There are a lot of tests of bisecting arrays of objects:

https://github.com/d3/d3-array/blob/main/test/bisector-test.js

How is your usage different? Can you share a minimal, complete, live example of how it is not doing what you expect, say as an Observable notebook?

@louking
Copy link
Author

louking commented Apr 8, 2022

see https://observablehq.com/@louking/d3-bisector-with-object-comparison-function

I'm not sure the best way to get observable to print like console.log() but the debugger brings you into where you can see the console.log()

the compare1(x,x) causes the bisector to always return hi

@mbostock
Copy link
Member

mbostock commented Apr 8, 2022

If you pass an accessor rather than a comparator,

bisectX = d3.bisector(d => d.x).left

then it works as intended.

https://observablehq.com/d/468b832f4f688643

But, I think you’re right that this might be a bug for bisector’s weird comparator that takes different arguments. I need to investigate a little more.

@mbostock mbostock reopened this Apr 8, 2022
@mbostock mbostock changed the title bisector compare of array of objects no longer works bisector no longer supports two-argument (object, value) comparator Apr 8, 2022
@mbostock
Copy link
Member

mbostock commented Apr 8, 2022

Also, it works if you implement a “normal” (symmetric, non-weird) comparator:

bisectX = d3.bisector((a, b) => a.x - b.x).left

And then you pass in test values that have the same shape as the things you are bisecting, e.g.

bisectX(dataarr, {x: 2})

However, this approach deviates from the example given in the README, and also deviates from the more recommended pattern of using an accessor (whereby you pass in a search value equivalent to the accessor’s output, not its input). So, I think at a minimum we need to update some documentation here, but ideally we find a way to make this work.

@mbostock
Copy link
Member

mbostock commented Apr 9, 2022

I have a fix up in #250. Thanks for the additional explanation that helped me understand the problem! 🙏

@louking
Copy link
Author

louking commented Apr 9, 2022

If you pass an accessor rather than a comparator,

bisectX = d3.bisector(d => d.x).left

then it works as intended.

I don't think this works for me, because my actual comparator handles both ascending and descending arrays.

            that.bisectX = d3.bisector(function (d, x) {
                if (that.xascending) {
                    return d.x - x;
                } else {
                    return x - d.x;
                }
            }).left;

In the meantime, to work with d3 7.4.2, I've created a parallel array with just the x values, and changed d.x in the above function to d.

Thanks for your support!

@mbostock
Copy link
Member

mbostock commented Apr 9, 2022

In the descending case, you could say

bisectX = d3.bisector(d => -d.x).left

and then pass -x as the search value. Or you can use the symmetric comparator as I described in the other comment. Here is the descending form:

bisectX = d3.bisector((a, b) => b.x - a.x).left

and then pass an object {x} to search.

@mbostock
Copy link
Member

mbostock commented Apr 9, 2022

So, to flesh out the example, you could use a symmetric comparator like so:

bisectX = d3.bisector(xascending ? (a, b) => a.x - b.x : (a, b) => b.x - a.x).left

And then say

bisectX(array, {x})

Or you could use an accessor like so:

bisectX = d3.bisector(xascending ? (d) => d.x : (d) => -d.x).left

And then say

bisectX(array, xascending ? x : -x)

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

Successfully merging a pull request may close this issue.

2 participants