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.

Sign up for GitHub

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

Fix invert point scale logic to get option closest to mouse pointer #150

Merged
merged 1 commit into from
Apr 29, 2019
Merged

Fix invert point scale logic to get option closest to mouse pointer #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 22 additions & 27 deletions src/utils/Scale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,28 +117,12 @@ export function scaleEqual(scaleA, scaleB) {
_.isEqual(scaleA.range(), scaleB.range());
}

export function indexOfClosestLeftNumberInList(number, sortedList) {
if (sortedList.length <= 1) {
return 0;
}

const isDescending = sortedList[0] > sortedList[1];
// _.sortedIndex works on ascending lists only
// so reverse if working with descending list
const listCopy = isDescending ? sortedList.slice().reverse() : sortedList;

// Get lower bound of where number falls
let indexForNumber = _.sortedIndex(listCopy, number);
if (isDescending && indexForNumber < sortedList.length) {
indexForNumber += 1;
} else if (!isDescending && indexForNumber > 0) {
indexForNumber -= 1;
}

return isDescending
? // If descending, remap indexForNumber to work with descending list
Math.abs(indexForNumber - sortedList.length)
: indexForNumber;
export function indexOfClosestNumberInList(number, list) {
return list.reduce((closestI, current, i) => {
return Math.abs(current - number) < Math.abs(list[closestI] - number)
? i
: closestI;
}, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for keeping the Reactochart dream alive!

Just FYI - the original reason i wrote it <-- that way was because we were assuming the input data would be sorted (by domain, either ascending or descending). With this assumption, you can use _.sortedIndex to do a binary search in O(log n) time. With ^the new method here, it works for all data regardless of sort order, but always searches every data point, so it's O(n) which can get slow for large datasets.

IIRC, the plan was to eventually allow a sorted flag (on the XYPlot i guess?) that would tell Reactochart whether the data was sorted or not - it would default false, but if true would use tricks like this to get better performance for sorted data. Might be a nice enhancement for the future.

Hope all is well with y'all :)

}

export function invertPointScale(scale, rangeValue) {
Expand All @@ -147,10 +131,21 @@ export function invertPointScale(scale, rangeValue) {
// shim until d3.scalePoint.invert() is implemented for real
// given a value from the output range, returns the *nearest* corresponding value in the input domain
const rangePoints = domain.map(domainValue => scale(domainValue));
const nearestLeftPointIndex = indexOfClosestLeftNumberInList(
rangeValue,
rangePoints
);

return domain[nearestLeftPointIndex];
if (rangePoints.length <= 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a use case where there is a 0? is the undefined caught in this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspecting d3's code domain looks like it'll always return an array https://github.com/d3/d3-scale/blob/master/src/continuous.js#L91

return domain[0];
}

const isDescending = rangePoints[0] > rangePoints[1];

// _.sortedIndex works on ascending lists only
// so reverse if working with descending list
if (isDescending) {
domain.reverse();
rangePoints.reverse();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I don't think you need this descending/reverse logic anymore since you're not assuming anything about sort order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point, will actually opt to update the code above and just use lodash's sortedIndex


const nearestPointIndex = indexOfClosestNumberInList(rangeValue, rangePoints);

return domain[nearestPointIndex];
}
4 changes: 2 additions & 2 deletions tests/jsdom/spec/utils.Axis.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe("Axis utils", () => {
outerX: 50,
outerY: 0,
xScale: scale,
xValue: "a"
xValue: "b"
});
});

Expand Down Expand Up @@ -213,7 +213,7 @@ describe("Axis utils", () => {
outerX: 0,
outerY: 50,
yScale: scale,
yValue: "a"
yValue: "b"
});
});
});
Expand Down
10 changes: 5 additions & 5 deletions tests/jsdom/spec/utils.Scale.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
initScale,
isValidScale,
invertPointScale,
indexOfClosestLeftNumberInList
indexOfClosestNumberInList
} from "../../../src/utils/Scale";

describe("Scale utils", () => {
Expand Down Expand Up @@ -83,10 +83,10 @@ describe("Scale utils", () => {
});
});

describe("indexOfClosestLeftNumberInList", () => {
it("returns index of left closest to the number in the array", () => {
expect(indexOfClosestLeftNumberInList(1.5, [5, 4, 3, 2, 1])).to.equal(3);
expect(indexOfClosestLeftNumberInList(1.5, [1, 2, 3, 4, 5])).to.equal(0);
describe("indexOfClosestNumberInList", () => {
it("returns index of closest to the number in the array", () => {
expect(indexOfClosestNumberInList(1.5, [5, 4, 3, 2, 1])).to.equal(3);
expect(indexOfClosestNumberInList(1.5, [1, 2, 3, 4, 5])).to.equal(0);
});
});

Expand Down