-
Notifications
You must be signed in to change notification settings - Fork 56
Divide and conquer traverse #54
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
Conversation
Fixes test of parent commit, 6eb97e5.
Also use a type synonym to simplify type signatures in Test.Main
src/Data/Traversable.js
Outdated
| case 1: return map(array1)(f(array[bot])); | ||
| case 2: return apply(map(array2)(f(array[bot])))(f(array[bot + 1])); | ||
| default: | ||
| var pivot = Math.floor((top + bot) / 2); |
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 reason for the trickier pivot selection was because e.g. for 6 elements, a pivot of 2 is superior to a pivot of 3. Can we preserve this?
cc @syaiful6
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.
Ah I see, thanks. So would it make sense to add a branch to our switch statement for when top - bot == 6? Can you explain the general principle behind the trickier pivot selection? I can't quite see how to modify your code to get it to work other than perhaps to leave it alone and add a branch to the switch statement for when top - bot == 3.
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.
By the way, please feel free to take these commits and put them back into your PR if you want, and then I could close this.
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.
Or at least the ones where I added more tests.
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 idea is simply to choose pivots that will yield two even-length partitions where possible.
I suppose the best approach would be to add a case 3. IIUC this is the only nonterminating case under #48; 4 will be divided into 2 and 2.
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.
@hdgarrood OK, I'll pull your changes back. Thanks for all the fixups.
Also add comment to explain why it is this way.
0391706 to
a251fde
Compare
8fc95fa to
e7cb46c
Compare
|
I've made the changes to pivot selection discussed earlier in this PR, and double checked that this is not a breaking change (note that we only changed |
|
Did we benchmark it at all in the end? |
|
We did, I added a comment in #48 with results. Performance wise it seems to be pretty much the same as the previous implementation, at least on V8. |
|
@hdgarrood I'm glad it worked; it didn't in my own tests. The asymptotics have several dimensions, each induced by particular applicatives. Some examples:
|
|
Resolved by #78 (!) |
Like #48, but rebased, and I fixed a minor error, and added some more tests that would have caught it. /cc @S11001001