Implement bisect #2971
Implement bisect #2971
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2971 +/- ##
=======================================
Coverage 87.58% 87.59%
=======================================
Files 235 236 +1
Lines 18800 18823 +23
Branches 4784 4787 +3
=======================================
+ Hits 16466 16488 +22
- Misses 2158 2159 +1
Partials 176 176
Continue to review full report at Codecov.
|
Riju19
force-pushed
the
implement-bisect
branch
from
8d9ae87
to
a70d1ad
Compare
src/test/unit/bisect.test.js
Outdated
| it('returns index of the first number greater than x, occuring after low', function() { | ||
| expect(bisectionRight(array, 4, 8)).toBe(8); | ||
| expect(bisectionRight(array, 3, 9)).toBe(9); | ||
| expect(bisectionLeft(array, 2, -100)).toBe(2); |
There was a problem hiding this comment.
Looking at this test, I wonder if we shouldn't guard against specifying bad low and high values.
For example, at the start of the function:
if (low < 0) {
// throw error
}The python implementation throws an error for this case: https://github.com/python/cpython/blob/3fc7080220b8dd2e1b067b3224879133d895ea80/Lib/bisect.py#L26-L27
But doesn't check other cases. Then maybe we should do the same, with a TypeError (this is the equivalent in JavaScript to the Python's ValueError), what do you think?
There was a problem hiding this comment.
I agree we should handle bad low and high values, because especially in case when low is greater than the array length or both low and high are greater, we are getting absurd values, trying to refer to an element with that large index.
I believe both low and high should be in the range of the array and anything apart from that should throw an error.
Thanks for the link to the python implementation, I will check it out and add the error handling aspect to this.
src/test/unit/bisect.test.js
Outdated
| it('returns 0 if all elements are greater than x', function() { | ||
| expect(bisectionLeft(array, -5)).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Could you please also add a test with a x that's bigger than all other numbers?
(same for bisectionRight).
It should return array.length in both cases.
src/components/shared/Reorderable.js
Outdated
| @@ -14,6 +13,8 @@ import { | |||
| extractDomRectValue, | |||
| } from 'firefox-profiler/utils/css-geometry-tools'; | |||
|
|
|||
| import { bisectionRight } from '../../utils/bisect'; | |||
There was a problem hiding this comment.
Can you please import using the alias firefox-profiler for consistency?
import { bisectionRight } from 'firefox-profiler/utils/bisect';
Riju19
force-pushed
the
implement-bisect
branch
2 times, most recently
from
6dcbb8b
to
b9a63d2
Compare
Export two functions using ES6 modules: bisectionLeft and bisectionRight. Add comments about the original source of this code and the description of the two functions. Add flow typing.
For the bisection functions, low and high values should lie within the range of the array. This commit adds a check to the bisection functions to throw TypeError in case the low or high values passed are negative or greater than the array length. This commit also adds relevant testcases to bisect.test.js to check these conditions.
Riju19
force-pushed
the
implement-bisect
branch
from
b9a63d2
to
6943c86
Compare
This pull request is to add an implementation of bisection under utils and remove the usage of bisection from the node-bisection library.
It is the first part of the issue #2139