-
Notifications
You must be signed in to change notification settings - Fork 6
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
Optimized search #32
Optimized search #32
Conversation
…solve even if they were not in the provided array
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.
This looks great apart from a minor remark. 🚢
var n = parameters.stops.length; | ||
if (n === 1) return parameters.stops[0][1]; | ||
if (input <= parameters.stops[0][0]) return parameters.stops[0][1]; | ||
if (input >= parameters.stops[n - 1][0]) return parameters.stops[n - 1][1]; |
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.
The edge cases are the same code in two places, so we can move it under the binarySearchForIndex
function to remove repetition.
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.
@mourner Good point-- it is repetitive. I can't see a clear way right now to clean it up without adding additional conditionals in evaluateExponentialFunction
to signal that we want to clamp the output and not interpolate if the edge cases are hit. The first thing I tried makes performance notably slower on case where we have few stops. Maybe you can offer a suggestion?
I tried this in evaluateExponentialFunction
:
// Let binarySearchForIndex() handle the edge cases
var index = binarySearchForIndex(parameters.stops, input);
// But make sure not to run interpolation on edge cases
if (index >= parameters.stops.length - 1)
return parameters.stops[parameters.stops.length - 1][1];
if (input <= parameters.stops[0][0])
return parameters.stops[0][1];
return interpolate(
input,
base,
parameters.stops[index][0],
parameters.stops[index + 1][0],
parameters.stops[index][1],
parameters.stops[index + 1][1]
);
and performance went down to:
>>> Evaluating exponential function for 1000000 iterations with 10 stops
Only include values within the domain of stops:
Time: 182.110ms
Include values outside the domain of stops:
Time: 97.764ms
vs about 156ms and 80ms before the change.
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, let's follow up with other PRs if we find any ways to improve.
Addresses #31 ("Improve function performance for an increasing number of stops"). Full performance results are in that ticket.
I wasn't sure if including
profile.js
in/tests/
is the right place for it-- just let me know if I should do something else there.cc/ @mourner