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

bug in nodes #7

Open
make-github-pseudonymous-again opened this issue Sep 6, 2015 · 5 comments · May be fixed by #10
Open

bug in nodes #7

make-github-pseudonymous-again opened this issue Sep 6, 2015 · 5 comments · May be fixed by #10

Comments

@make-github-pseudonymous-again

It there a reason why it is assumed that xs.length <= 8 here? There is no such assumption in Hinze and Paterson.

const FingerTree = require( 'fingertree' ) ;
let X = FingerTree.fromArray( [ ] ) ;
for ( let i = 0 ; i < ( 4 * 4 + 4 ) ; ++i ) X = X.addLast( i ) ;
let Y = FingerTree.fromArray( [ ] ) ;
for ( let i = 0 ; i < ( 4 * 4 + 4 ) ; ++i ) Y = Y.addFirst( i ) ;
X.concat( Y ).measure( )
Error: invalid number of nodes
    at nodes (/home/aureooms/node_modules/fingertree/src/fingertree.js:994:22)
    at DelayedFingerTree.thunk (/home/aureooms/node_modules/fingertree/src/fingertree.js:963:35)
    at DelayedFingerTree.force (/home/aureooms/node_modules/fingertree/src/fingertree.js:794:24)
    at DelayedFingerTree.measure (/home/aureooms/node_modules/fingertree/src/fingertree.js:810:17)
    at FingerTree.Deep.measure (/home/aureooms/node_modules/fingertree/src/fingertree.js:576:52)
    at DelayedFingerTree.measure (/home/aureooms/node_modules/fingertree/src/fingertree.js:810:25)
    at FingerTree.Deep.measure (/home/aureooms/node_modules/fingertree/src/fingertree.js:576:52)
    at repl:1:15
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
@zot
Copy link
Contributor

zot commented Sep 7, 2015

I believe the fix is to define the nodes() function like it is on page 8 of the paper. I have updated my CoffeeScript version like this:

    nodes = (m, xs, res)->
      res = res ? []
      switch xs.length
        when 2 then res.push new Node(m, xs)
        when 3 then res.push new Node(m, xs)
        when 4 then res.push new Node(m, [xs[0], xs[1]]), new Node(m, [xs[2], xs[3]])
        else
          res.push new Node m, [xs[0], xs[1], xs[2]]
          nodes m, xs.slice(3), res
      res

Note that I added the accumulator res to avoid linear array insert behavior

@make-github-pseudonymous-again
Copy link
Author

Note that I added the accumulator res to avoid linear array insert behavior

You mean quadratic?

Is there a bound on the number of nodes returned by this procedure?

@zot
Copy link
Contributor

zot commented Sep 7, 2015

Last time I calculated array insert behavior, it was O(n) but that was a long time ago, has it changed to quadratic now? ;)

@make-github-pseudonymous-again
Copy link
Author

I misunderstood. You meant "avoid linear behaviour when inserting a single element (using concat I suppose, which produces a complete copy of the original array) by using push, which updates the array using at most linear time but amortized constant time".
While I meant "avoid quadratic behaviour when inserting n elements..." which is the same.

@epixode epixode linked a pull request Nov 17, 2016 that will close this issue
make-github-pseudonymous-again pushed a commit to functional-data-structure/finger-tree that referenced this issue Jul 17, 2017
@make-github-pseudonymous-again
Copy link
Author

Sorry to come back so late but we should have xs.length <= 12 (hint: solve x = ceil(x/3) + 8) so the implementation does not have to handle all lengths.

EDIT: See end of Section 3.3 of Hinze and Paterson (Finger trees: a simple general-purpose data structure) page 9 (paragraph starting with "It is easy to check..." and Exercise 2).

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

Successfully merging a pull request may close this issue.

2 participants