djbpitt / scala-graphing-sandbox

2 stars 0 forks source link

Add test for end value of 0 in lcp array #1

Closed djbpitt closed 1 year ago

djbpitt commented 1 year ago

Currently don't test for situation where value goes higher and then goes to zero. We now test explicitly for > 0. If we remove that test we fear that it would create a block with 0, which would be incorrect. If the current code (and this account) is correct, our comment needs to be amended. We don't yet have a test to prove it.

rhdekker commented 1 year ago

Added a test with a zero at the end of a number of non zero LCP values. Current implementation works as expected. See commit #17fbbfc Removing the lcp_value larger than 0 check in the if and replacing the != in the last condition with an < operator makes all the tests fail. That result is not surprising, the test environment allows us to settle that issue. We can discuss and amend the comment during our next meeting.

rhdekker commented 1 year ago

Keeping the lcp_value > 0 condition in the if and changing the final condition from openIntervals.top.length != lcp_value to openIntervals.top.length < lcp_value also works. This makes sense as in this part of the code the value can never higher and we should not create a block is the value is the same (that would open an extra block for a block that is already open). So the only valid case to open a new block is when the lcp_value is lower. It might be preferable to make that explicit in the code by accepting the "<" operator change.

djbpitt commented 1 year ago

Done!