search

lumean / svg-graph2

SVG:::Graph is a pure Ruby library for generating charts, which are a type of graph where the values of one axis are not scalar. SVG::Graph has a verry similar API to the Perl library SVG::TT::Graph, and the resulting charts also look the same. This isn't surprising, because SVG::Graph started as a loose port of SVG::TT::Graph.
Other
50 stars 20 forks source link

Fix bug in Plot which enabled too-short axis #10

Closed ashleydavies closed 5 years ago

ashleydavies commented 5 years ago

This fixes a bug in Plot which allowed the axis to be insufficiently large to contain the last data point in the event that the last data point does not divide evenly min_value + n*step.

Demo of problem:

irb(main):041:0> 5.step(10, 2) {|x|puts x}
5
7
9

.step never goes above the upper bound

Example graph:

screenshot 2019-01-31 at 21 08 06
ashleydavies commented 5 years ago

@lumean I haven't tested the repo with this change -- I just copy/pasted the bits I changed and ran them in irb with concrete values to check the syntax isn't wrong. I haven't got an environment handy to test this with so if you have something convenient it'd be great if you could verify this doesn't break everything!

lumean commented 5 years ago

Sure, thanks for reporting the issue.

lumean commented 5 years ago

Just realized there are some more issues with plot. i.e. following parameters are not correctly honored:

      :max_x_value 
      :max_y_value
      :min_x_value
      :min_y_value

will discard this PR, since a more extensive fix is needed.

ashleydavies commented 5 years ago

Ah I hadn’t noticed — happy to send up a PR fixing those too this weekend if it saves you some effort?

ashleydavies commented 5 years ago

Just saw you’ve already done a patch for some of it — thanks! Let me know if I can help in any way.

I think (haven’t tested, so apologies if I’m missing anything) the new step lines might have a new (much more minor) bug where if the last point on the axis does perfectly divide the step then it’ll add an extra point, e.g. step 2 from 6 to 10 will do 6,8,10,12, since you unconditionally add step onto the end value?

lumean commented 5 years ago

yes, that was by intention. It was just a quick test to see if the unit test I added was effective in reproducing the short axis issue. See my changes in test_plot.rb in the last commit.If you have the time and would like to, sure I would appreciate your contributions a lot.regarding testing, so far I didn't find a really good way to test if a generated graph lookd as expected. In most cases I just run the unit tests and examples and then look at the generated files in a browser.  If you have some ideas how to automate this process in the unit tests, would also appreciate to hear your inputs. e.g. selenium, jpeg diff, diff of generated svg files, etc.

ashleydavies commented 5 years ago

Ah I see - thanks for the clarification!

For testing, one approach I can think of is testing properties in the SVG, e.g. exists a line with slope 1 between the edges of the x axis and y axis. I guess diffing works too, but it might be a bit fragile to subtle visual changes which don’t introduce any incorrectness. Happy to crank out a few tests this weekend if you think that sounds sensible.