Fixing monotonic line plot issue #32

[nanobowers]

@mrkn and @nakilon

Fixing #32

The fix was related to calculating the Y and Y-offset independently, with Y being calculated with floating point math . Now Offset is computed first with integer math (modulus), then that value is used in an integer divide.

As part of this fix, the 1-based indexing from Julia has been replaced with 0-based ruby array indexing which simplifies the code. In other words, the repeated use of +1 and -1 has been removed.

Additionally a file to validate the fix has been added into the examples directory, though perhaps this should be a unit test?

[mrkn]

@nanobowers Could you please add a test case to examine this problem in test-lineplot.rb?

[nanobowers]

@mrkn Updated repo to add test case.

[Nakilon]

I remember when I saw that +1-1 code I wasn't brave enough to figure it out ..D Thank you for fixing it! Can't test right now but I'm pretty sure you've tested it well. Basically it's about random changing the width and height and see if it's broken. Theoretically there can be a test that generates a monotonous sequence (for example a sort of sinus function with always non-negative derivative) (or non-positive for the down direction) and tries different width and height checking that the Brail subset of the characters in each line follow the order.

[nanobowers]

Hi @Nakilon , I am fairly confident that this fix will work for all cases and have added your example as a testcase that displays correctly. Monotonicity of the output image is somewhat difficult to validate generically without either

• writing a complex wrapper to validate how the braille is displayed or
• poking into the plot's Canvas object prior to calling #render , however accessing the canvas is not part of the public API.

But anyway, i think the change (taking the integer-modulo, then subtracting and integer dividing) should be safer and perhaps provably so. I am hopeful that the fix is both sufficient, but will let @mrkn advise on if further testcases are warranted.

If you wish to try out the fix and have the means to do so, please feel free to clone: https://github.com/nanobowers/unicode_plot.rb/tree/issue32 and run this script in the example directory. https://github.com/nanobowers/unicode_plot.rb/blob/issue32/example/issue32.rb

Cheers and Happy New Year.

[mrkn]

@nanobowers I want to keep the calculation core uses 1-origin to sync the logic to UnicodePlots.jl. Could you change your patch to keep 1-origin?

[nanobowers]

@mrkn I have reverted the logic to use 1-origin for compatiblity with Julia, which makes the diff much smaller and squashed my merges down, this PR still should include the fix and tests.

[mrkn]

@nanobowers Thank you very much!!