gopherdata / gophernotes

The Go kernel for Jupyter notebooks and nteract.
MIT License
3.8k stars 264 forks source link

Error in gonum.org/v1/plot code that used to work #198

Open vsivsi opened 4 years ago

vsivsi commented 4 years ago

Hi, After not using gophernotes for a month or so, something has changed so that example code for gonum.org/v1/plot no longer runs properly in gophernotes, but continues to run fine in the latest gomacro.

Reproduction from this example code:

import (
    "log"
    "image/color"
    "gonum.org/v1/plot"
    "gonum.org/v1/plot/plotter"
    "gonum.org/v1/plot/plotutil"
    "gonum.org/v1/plot/vg"
)

groupA := plotter.Values{20, 35, 30, 35, 27}
groupB := plotter.Values{25, 32, 34, 20, 25}
groupC := plotter.Values{12, 28, 15, 21, 8}
groupD := plotter.Values{30, 42, 6, 9, 12}

p, err := plot.New()
if err != nil {
    log.Panic(err)
}
p.Title.Text = "Bar chart"
p.Y.Label.Text = "Heights"

w := vg.Points(8)

barsA, err := plotter.NewBarChart(groupA, w)
if err != nil {
    log.Panic(err)
}
barsA.Color = color.RGBA{R: 255, A: 255}
barsA.Offset = -w / 2

barsB, err := plotter.NewBarChart(groupB, w)
if err != nil {
    log.Panic(err)
}
barsB.Color = color.RGBA{R: 196, G: 196, A: 255}
barsB.Offset = w / 2

barsC, err := plotter.NewBarChart(groupC, w)
if err != nil {
    log.Panic(err)
}
barsC.XMin = 6
barsC.Color = color.RGBA{B: 255, A: 255}
barsC.Offset = -w / 2

barsD, err := plotter.NewBarChart(groupD, w)
if err != nil {
    log.Panic(err)
}
barsD.Color = color.RGBA{B: 255, R: 255, A: 255}
barsD.XMin = 6
barsD.Offset = w / 2

p.Add(barsA, barsB, barsC, barsD)
p.Legend.Add("A", barsA)
p.Legend.Add("B", barsB)
p.Legend.Add("C", barsC)
p.Legend.Add("D", barsD)
p.Legend.Top = true
p.NominalX("Zero", "One", "Two", "Three", "Four", "",
    "Six", "Seven", "Eight", "Nine", "Ten")

p.Add(plotter.NewGlyphBoxes())
err = p.Save(300, 250, "barChart2.png")
if err != nil {
    log.Panic(err)
}

Which panics with:

type <gonum.org/v1/plot.Legend>: inconsistent 0-th method signature:
    go/types.Type has receiver <var  *gonum.org/v1/plot.Legend> and 2 parameters: func(string, ...gonum.org/v1/plot.Thumbnailer)
    reflect.Type has 2 parameters: func(plot.Legend, string) vg.Length

The code above works fine on the latest commit of gomacro by itself, I only see this failure in a fresh install of gophernotes.

I've tried clearing all of the state in ~/go/pkg and ~/go/src/gomacro.imports but that didn't resolve anything. Has gophernotes fallen behind gomacro in some respect and this is a symptom?

vsivsi commented 4 years ago

FYI:

plot.Legend.Add() is variadic for the plot.Thumbnailer. See definition

And that seems to be the difference between what go/types.Type and reflect.Type are seeing.

Still unclear why there is a difference between gomacro and gophernotes.

cosmos72 commented 4 years ago

One possible difference is that gophernotes no longer includes a vendored copy of gomacro. Instead, it relies on its go.mod and modules support in Go toolchain to download the correct version of gomacro.

This may result in using a possibly outdated version of gomacro in any of the following cases:

  1. gophernotes is built with GO111MODULE=off
  2. GO111MODULE is not set, gophernotes is downloaded inside GOPATH, and built from there with Go 1.11 (which ignores go.mod inside GOPATH when GO111MODULE is not set or set to off)

That's why the installation procedure contains an explicit

env GO111MODULE=on go get github.com/gopherdata/gophernotes

To check if that's what happened in your case, you can clean your home directory with

go clean
go clean -cache
go clean -modcache

then download and compile gophernotes with

env GO111MODULE=on go get -v github.com/gopherdata/gophernotes

examine the output to find which version of gomacro gets downloaded, then proceed with the rest of the installation as usual (copying files into ~/.local/share/jupyter/kernels/gophernotes).

If this does not solve your issue, please post the output of the installation procedure, and I will investigate further.

vsivsi commented 4 years ago

Hi, thanks for your quick response, as always.

So I think there are two different issues here. The first was some stale go mod cache issues as you pointed out above. To get past that, I've installed all of this on a fresh macOS laptop that has never had any of this installed. The result is that gomacro is now failing in the same way as gophernotes. So that mystery is solved. But that failure remains.

Here is a simpler reproduction of the issue.

This go program runs perfectly (go run plot.go) and generates the output shown below:

package main
import (
        "gonum.org/v1/plot"
        "gonum.org/v1/plot/plotter"
        "gonum.org/v1/plot/vg"
        "image/color"
        "log"
)

func main() {
        groupA := plotter.Values{20, 35, 30, 35, 27}

        p, err := plot.New()
        if err != nil {
                log.Panic(err)
        }
        p.Title.Text = "Bar chart"
        p.Y.Label.Text = "Heights"

        w := vg.Points(8)

        barsA, err := plotter.NewBarChart(groupA, w)
        if err != nil {
                log.Panic(err)
        }
        barsA.Color = color.RGBA{R: 255, A: 255}
        barsA.Offset = -w / 2

        p.Add(barsA)
        p.Legend.Add("A", barsA)
        err = p.Save(300, 250, "barChart.png")
        if err != nil {
                log.Panic(err)
        }
}

barChart

However, the same code (with the package and func main stripped) fails in gophernotes and gomacro in the same way as the OP above:

$ ~/go/bin/gomacro -i plotfmt.go 
// debug: looking for package "gonum.org/v1/plot" ...
// debug: compiling "/Users/vsi/go/src/gomacro.imports/gonum.org/v1/plot/plot.go" ...
go: finding gonum.org/v1/plot latest
// debug: looking for package "gonum.org/v1/plot/plotter" ...
// debug: compiling "/Users/vsi/go/src/gomacro.imports/gonum.org/v1/plot/plotter/plotter.go" ...
go: finding gonum.org/v1/plot latest
// debug: looking for package "gonum.org/v1/plot/vg" ...
// debug: compiling "/Users/vsi/go/src/gomacro.imports/gonum.org/v1/plot/vg/vg.go" ...
go: finding gonum.org/v1/plot latest
// warning: redefined identifier: err
plotfmt.go:28:8: cannot use <*gonum.org/v1/plot/plotter.BarChart> as <gonum.org/v1/plot.Plotter> in argument to p.Add
plotfmt.go:29:20: cannot use <*gonum.org/v1/plot/plotter.BarChart> as <gonum.org/v1/plot.Thumbnailer> in argument to p.Legend.Add
// GOMACRO, an interactive Go interpreter with generics and macros
// Copyright (C) 2018-2019 Massimiliano Ghilardi <https://github.com/cosmos72/gomacro>
// License MPL v2.0+: Mozilla Public License version 2.0 or later <http://mozilla.org/MPL/2.0/>
// This is free software with ABSOLUTELY NO WARRANTY.
//
// Type :help for help
gomacro> barsA
&{[20 35 30 35 27] 8 {255 0 0 255} {{0} 1 [] 0} -4 0 false <nil>}   // *gonum.org/v1/plot/plotter.BarChart
gomacro> barsA.Plot
type <gonum.org/v1/plot/plotter.BarChart>: inconsistent 3-th method signature:
    go/types.Type has receiver <var  *gonum.org/v1/plot/plotter.BarChart> and 2 parameters: func(gonum.org/v1/plot/vg/draw.Canvas, *gonum.org/v1/plot.Plot)
    reflect.Type has 1 parameters: func(*plotter.BarChart) (float64, float64, float64, float64)
gomacro> var pp plot.Plotter
gomacro> pp = barsA
repl.go:1:1: error compiling assignment: pp = barsA
    type <gonum.org/v1/plot/plotter.BarChart>: inconsistent 3-th method signature:
    go/types.Type has receiver <var  *gonum.org/v1/plot/plotter.BarChart> and 2 parameters: func(gonum.org/v1/plot/vg/draw.Canvas, *gonum.org/v1/plot.Plot)
    reflect.Type has 1 parameters: func(*plotter.BarChart) (float64, float64, float64, float64)
gomacro> 

Here's the code in plotfmt.go:

import (
     "gonum.org/v1/plot"
     "gonum.org/v1/plot/plotter"
     "gonum.org/v1/plot/vg"
     "image/color"
     "log"
)
groupA := plotter.Values{20, 35, 30, 35, 27}

p, err := plot.New()
if err != nil {
     log.Panic(err)
}
p.Title.Text = "Bar chart"
p.Y.Label.Text = "Heights"

w := vg.Points(8)

barsA, err := plotter.NewBarChart(groupA, w)
if err != nil {
     log.Panic(err)
}
barsA.Color = color.RGBA{R: 255, A: 255}
barsA.Offset = -w / 2

p.Add(barsA)
p.Legend.Add("A", barsA)
err = p.Save(300, 250, "barChart.png")
if err != nil {
     log.Panic(err)
 }

As I said in the OP, this all used to work fine, so something in gomacro/gophernotes seems to have recently changed to break this case. The only clue I can offer is it seems to have something to do with recognizing variadic methods as matching interfaces.

Thanks for looking into it!

vsivsi commented 4 years ago

Here's a gist with the same code + failure in gophernotes:

https://gist.github.com/vsivsi/30f8bd862c1faef7a7710764b98d80b0

cosmos72 commented 4 years ago

Thanks for the detailed report :) I will investigate it as soon as possible.

Well, at least gophernotes and gomacro have the same behavior now, although it's wrong.

An unrelated note: there's no need to strip package main and func main() { /*...*/ } from source code fed to gophernotes or gomacro - they are both supposed to handle it correctly. You only need to add main() at the end, in order to actually run the function main()

vsivsi commented 4 years ago

Hi, @cosmos72 I hope you are staying safe and healthy! Just checking in to see if there's any progress on this issue or anything I can do to help. I've thought about doing a binary search in the commit history to at least narrow down when this broke and what change(s) caused it. Would that be helpful to you?

cosmos72 commented 4 years ago

I am fine, thanks! Stuck at home though, until the situation in Italy improves...

Yes, a binary search would help a lot, thanks for the offer!

If you do it, please use gomacro git repository - using gophernotes would just land you on one of the few commits where gomacro is updated massively (tens of commits are squashed to a single one)

vsivsi commented 4 years ago

Yes, we are also stuck at home, so time to find some bugs!

Ok, I followed your advice and did this work in gomacro only, and have been able to trace the problem to the commit when module support was added: https://github.com/cosmos72/gomacro/commit/caa3eae00fe6400ce51b2a0b7d124df0debe0e72

Prior to that, the test code above worked fine, but of course the plot libraries had to be separately installed in the GOPATH. i.e. go get gonum.org/v1/plot/...

And since that commit, the test code continues to work fine, so long as the plot library is installed in the GOPATH, even when using the new module support.

But if the plot libs have never been installed in the GOPATH, then the repro code above fails, for every commit since the module support was added.

I have verified this using the official golang Docker container for go 1.13.9 I used Docker for this to eliminate any possibility of golang installation state (caches, etc.) confusing the issue. I stayed with go 1.13 for the analysis because I'm aware that 1.14 surfaced new issues with modules that would have confounded this analysis.

So, for example, if you start a container with the official go release from Dockerhub:

docker run -it -v `pwd`:`pwd` -w `pwd` golang:1.13

And then in the container run:

go get github.com/cosmos72/gomacro
gomacro -i plotfmt.go

You get:

// debug: looking for package "gonum.org/v1/plot" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/plot.go" ...
// debug: looking for package "gonum.org/v1/plot/plotter" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/plotter/plotter.go" ...
// debug: looking for package "gonum.org/v1/plot/vg" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/vg/vg.go" ...
// warning: redefined identifier: err
type <gonum.org/v1/plot.Legend>: inconsistent 0-th method signature:
    go/types.Type has receiver <var  *gonum.org/v1/plot.Legend> and 2 parameters: func(string, ...gonum.org/v1/plot.Thumbnailer)
    reflect.Type has 2 parameters: func(plot.Legend, string) vg.Length
// Welcome to gomacro. Type :help for help, :copyright for copyright and license.
// This is free software with ABSOLUTELY NO WARRANTY.
gomacro> 

Which reproduces this issue. But if you install the plotting library in the GOPATH first:

go get github.com/cosmos72/gomacro
go get gonum.org/v1/plot/...
gomacro -i plotfmt.go

You get the output below and the plot image is properly written to the output file:

// debug: looking for package "gonum.org/v1/plot" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/plot.go" ...
// debug: looking for package "gonum.org/v1/plot/plotter" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/plotter/plotter.go" ...
// debug: looking for package "gonum.org/v1/plot/vg" ...
// debug: compiling "/go/src/gomacro.imports/gonum.org/v1/plot/vg/vg.go" ...
// warning: redefined identifier: err
// Welcome to gomacro. Type :help for help, :copyright for copyright and license.
// This is free software with ABSOLUTELY NO WARRANTY.

You get the same behavior with both the first commit supporting modules and the current latest one, so this issue has remained static since Nov 18, 2019 when it first appeared.

https://github.com/cosmos72/gomacro/commit/caa3eae00fe6400ce51b2a0b7d124df0debe0e72 https://github.com/cosmos72/gomacro/commit/30b9859a00de0ba04d3ee9c57b029f8aaa1cc8dd

It is curious that having the plotting libraries in the GOPATH has any effect on gomacro when the module mode is engaged, as it appears to download and build them the same either way, but it seems that the dependencies between them are not being resolved by the module loader in the same way, depending on the contents of the GOPATH.

It is also the case that if you disable module support in gomacro with :option Import.Uses.Module that it continues to work if the libraries are installed, as you would expect.

So this problem was hidden to me for a time, because I had installed plotting libraries from the pre-module days. But when I moved to a new computer and left that state behind, I started seeing failures.

cosmos72 commented 4 years ago

Sigh. I really hoped it wasn't modules support, because golang.org/x/tools/go/packages is a huge black box... it will be painful.

vsivsi commented 4 years ago

Sorry to hear that. It seems for now I should just proceed with the modules support disabled.

Please let me know if there's anything more I can do to help.

cosmos72 commented 3 years ago

It seems the last round of bugfix for modules support in gomacro did the trick :)

the above code (with a minor change, see comment below) now works also in the latest version (0.7.2) of gophernotes - at least for me:

import (
     "gonum.org/v1/plot"
     "gonum.org/v1/plot/plotter"
     "gonum.org/v1/plot/vg"
     "image/color"
     "log"
)
groupA := plotter.Values{20, 35, 30, 35, 27}

p := plot.New() // plot.New() no longer returns an error as second value
p.Title.Text = "Bar chart"
p.Y.Label.Text = "Heights"

w := vg.Points(8)

barsA, err := plotter.NewBarChart(groupA, w)
if err != nil {
     log.Panic(err)
}
barsA.Color = color.RGBA{R: 255, A: 255}
barsA.Offset = -w / 2

p.Add(barsA)
p.Legend.Add("A", barsA)
err = p.Save(300, 250, "barChart.png")
if err != nil {
     log.Panic(err)
 }

@vsivsi can you confirm it works for you too?