guptarohit / asciigraph

Go package to make lightweight ASCII line graph ╭┈╯ in command line apps with no other dependencies.
https://pkg.go.dev/github.com/guptarohit/asciigraph
BSD 3-Clause "New" or "Revised" License
2.64k stars 99 forks source link

PlotMany() modifies caller data slices, yielding incorrect plots when called multiple times when a fixed Width is set #54

Closed vsivsi closed 1 month ago

vsivsi commented 3 months ago

Hi, this is a neat little library, thanks for developing it!

However I've encountered some surprising behavior by PlotMany() that can lead to wildly incorrect output graphs if not guarded against. Thanks for taking a look at this and any fixes you can make.

What I observe:

Repeated calls to PlotMany() with the same data slices (with new values appended for each new call) result in the passed data slices being unexpectedly modified by the method.

This appears to happen when a fixed Width() is specified, here: https://github.com/guptarohit/asciigraph/blob/master/asciigraph.go#L30-L39

This results in incorrect distortions in the output plots (the lines should be straight), such as:

 996 ┤                                                                                              ╭────
 946 ┤                                                                                     ╭────────╯
 896 ┤                                                                             ╭───────╯
 847 ┤                                                                     ╭───────╯
 797 ┤                                                               ╭─────╯
 747 ┤                                                         ╭─────╯
 697 ┤                                                   ╭─────╯
 647 ┤                                              ╭────╯
 598 ┤                                          ╭───╯
 548 ┤                                      ╭───╯
 498 ┤                                  ╭───╯
 448 ┤                               ╭──╯
 398 ┤                            ╭──╯
 349 ┤                         ╭──╯                                                                 ╭────
 299 ┤                       ╭─╯                                           ╭────────────────────────╯
 249 ┤                     ╭─╯                           ╭─────────────────╯
 199 ┤                   ╭─╯                ╭────────────╯
 149 ┤                 ╭─╯        ╭─────────╯
 100 ┤               ╭─╯   ╭──────╯
  50 ┤            ╭──╭─────╯
   0 ┼───────────────╯
                                  Default: reuse data slices (observed output)

What I expect:

Without very clear documentation, I expect that library API calls will not modify the length or contents of passed data slices. In the case of this library, there is no reason to expect that the data passed in for plotting will be modified, as that is not typical behavior for plotting packages.

At a minimum the documentation should be updated to warn users of this side-effect. But ideally, the library would make internal copies of data that needs to be modified.

The expected output (straight lines) is:

 996 ┤                                                                                                ╭──
 946 ┤                                                                                           ╭────╯
 896 ┤                                                                                      ╭────╯
 847 ┤                                                                                 ╭────╯
 797 ┤                                                                            ╭────╯
 747 ┤                                                                       ╭────╯
 697 ┤                                                                  ╭────╯
 647 ┤                                                             ╭────╯
 598 ┤                                                        ╭────╯
 548 ┤                                                   ╭────╯
 498 ┤                                              ╭────╯
 448 ┤                                          ╭───╯
 398 ┤                                     ╭────╯
 349 ┤                                ╭────╯                                                          ╭──
 299 ┤                           ╭────╯                                                ╭──────────────╯
 249 ┤                      ╭────╯                                      ╭──────────────╯
 199 ┤                 ╭────╯                            ╭──────────────╯
 149 ┤            ╭────╯                   ╭─────────────╯
 100 ┤       ╭────╯         ╭──────────────╯
  50 ┤  ╭────╭──────────────╯
   0 ┼───────╯
                             Workaround: freshly copy data slices (expected output)

Reproduction:

import (
    "fmt"
    "github.com/guptarohit/asciigraph"
)

func main() {
    data := [][]float64{nil, nil}
    data2 := [][]float64{nil, nil}
    dataCopy := [][]float64{nil, nil}

    for x := 1; x < 1000; x += 5 {
        data[0] = append(data[0], float64(x))
        data[1] = append(data[1], float64(x)/3.0)
        data2[0] = append(data2[0], float64(x))
        data2[1] = append(data2[1], float64(x)/3.0)

        dataCopy[0] = make([]float64, len(data[0]))
        copy(dataCopy[0], data[0])
        dataCopy[1] = make([]float64, len(data[1]))
        copy(dataCopy[1], data[1])

        _ = asciigraph.PlotMany(dataCopy, asciigraph.Height(20), asciigraph.Width(100), asciigraph.LowerBound(0))
        _ = asciigraph.PlotMany(data2, asciigraph.Height(20), asciigraph.Width(100), asciigraph.LowerBound(0))
    }

    fmt.Println(asciigraph.PlotMany(data, asciigraph.Height(20), asciigraph.Width(100), asciigraph.LowerBound(0), asciigraph.Caption("Workaround: freshly copy data slices (expected output)")))
    fmt.Println("\n")
    fmt.Println(asciigraph.PlotMany(data2, asciigraph.Height(20), asciigraph.Width(100), asciigraph.LowerBound(0), asciigraph.Caption("Default: reuse data slices (observed output)")))
}
guptarohit commented 1 month ago

Hi @vsivsi,

Thanks for notifying about this issue, I've fixed it in recent release, please do try it out.

Cheers ✨