plot: plots fails to render with oversized glyphs about plot

[sbinet]

it's coming from padY:
https://github.com/gonum/plot/blob/342a5ce/plot.go#L313...L333

func padY(p *Plot, c draw.Canvas) draw.Canvas {
	glyphs := p.GlyphBoxes(p)
	b := bottomMost(&c, glyphs)
	yAxis := verticalAxis{p.Y}
	glyphs = append(glyphs, yAxis.GlyphBoxes(p)...)
	t := topMost(&c, glyphs)

	miny := c.Min.Y - b.Min.Y
	maxy := c.Max.Y - (t.Min.Y + t.Size().Y)
	by := vg.Length(b.Y)
	ty := vg.Length(t.Y)
	n := (by*maxy - ty*miny) / (by - ty)
	m := ((by-1)*maxy - ty*miny + miny) / (by - ty)
	return draw.Canvas{
		Canvas: vg.Canvas(c),
		Rectangle: vg.Rectangle{
			Min: vg.Point{Y: n, X: c.Min.X},
			Max: vg.Point{Y: m, X: c.Max.X},
		},
	}
}

for the dimensions you chose, by and ty are equal( 0.5) (actually, it's even worse: b == t)
so both n and m are +Inf.

(the same issue may also happen in padX)

@kortschak what would be the regularization for this singularity ?

[kortschak]

I don't see how we can regularise this; we are being asked to fit an elephant into a matchbox, which is not something we can do. We probably should return a meaningful error here though. There could be a check in (*Plot).WriterTo for elements being oversized for the output. Ideally this would be returned from Draw since it would mean that users would see the errors too if they don't use WriterTo in their code, but that method doesn't return an error, so we can't do that.

[kortschak]

One possibility would be to add an Err method to Plot that Draw can communicate through. It's not ideal, but it is backwards compatible and at least allows discovery of failures during plot drawing.

[sbinet]

Or:

• break API (and add 'error' to 'Draw')
• return the big glyphbox in pady (and let the matchbox be exploded by the elephant)
• panic in pady/padx ? These are non recoverable errors that stem from user error.

All options (except the exploding matchbox one) suffer from the fact that the error/panic will be hard to link back to the problematic plotter.

[kortschak]

Another would be to draw only the glyph boxes in the case that there is an overflow (filled so that if it is completely overflowed it shows up). This at least gives an indication that something is wrong to look for.

[sbinet]

I am not sure I understand what you mean

[kortschak]

If we render obviously wrong red blocks on the plot instead of what the user is expecting, the positions of the red blocks will help them figure out which part caused the failure.

[sbinet]

if we just return c unmodified (in padY) under the elephant condition, we get the attached plot.

(which clearly looks bonkers)

alternatively: we could introduce a private plot.*Plot.draw method that returns an error and use that inside plot.*Plot.Save:

diff --git a/plot.go b/plot.go
index be19dd7..5881928 100644
--- a/plot.go
+++ b/plot.go
@@ -5,8 +5,10 @@
 package plot
 
 import (
+       "fmt"
        "image/color"
        "io"
        "math"
        "os"
        "path/filepath"
@@ -138,11 +140,18 @@ func (p *Plot) Add(ps ...Plotter) {
 // Draw draws a plot to a draw.Canvas.
 //
 // Plotters are drawn in the order in which they were
-// added to the plot.  Plotters that  implement the
+// added to the plot.  Plotters that implement the
 // GlyphBoxer interface will have their GlyphBoxes
 // taken into account when padding the plot so that
 // none of their glyphs are clipped.
 func (p *Plot) Draw(c draw.Canvas) {
+       err := p.draw(c)
+       if err != nil {
+               panic(err)
+       }
+}
+
+func (p *Plot) draw(c draw.Canvas) error {
        if p.BackgroundColor != nil {
                c.SetColor(p.BackgroundColor)
                c.Fill(c.Rectangle.Path())
@@ -165,21 +174,51 @@ func (p *Plot) Draw(c draw.Canvas) {
        ywidth := y.size()
 
        xheight := x.size()
-       x.draw(padX(p, draw.Crop(c, ywidth, 0, 0, 0)))
-       y.draw(padY(p, draw.Crop(c, 0, 0, xheight, 0)))
+       cx, err := padX(p, draw.Crop(c, ywidth, 0, 0, 0))
+       if err != nil {
+               return err
+       }
+       x.draw(cx)
+
+       cy, err := padY(p, draw.Crop(c, 0, 0, xheight, 0))
+       if err != nil {
+               return err
+       }
+       y.draw(cy)
+
+       cc, err := padX(p, draw.Crop(c, ywidth, 0, xheight, 0))
+       if err != nil {
+               return err
+       }
+
+       dc, err := padY(p, cc)
+       if err != nil {
+               return err
+       }
 
-       dataC := padY(p, padX(p, draw.Crop(c, ywidth, 0, xheight, 0)))
        for _, data := range p.plotters {
-               data.Plot(dataC, p)
+               data.Plot(dc, p)
        }
 
-       p.Legend.Draw(draw.Crop(c, ywidth, 0, xheight, 0))
+       err = p.Legend.draw(draw.Crop(c, ywidth, 0, xheight, 0))
+       if err != nil {
+               return fmt.Errorf("could not draw legend: %w", err)
+       }
+       return nil
 }
@@ -503,10 +567,17 @@ func (p *Plot) NominalY(names ...string) {
 //   - .tif|.tiff
 func (p *Plot) WriterTo(w, h vg.Length, format string) (io.WriterTo, error) {
        c, err := draw.NewFormattedCanvas(w, h, format)
        if err != nil {
                return nil, err
        }
-       p.Draw(draw.New(c))
+       err = p.draw(draw.New(c))
+       if err != nil {
+               return nil, fmt.Errorf("could not draw: %w", err)
+       }
        return c, nil
 }

that said, there's a precedent (in plot.Legend.Draw) to panic if we are asked to do something silly or simply not doable.

I guess I'd err on just panicking (and also introducing the plot.*Plot.draw method).

(even if I remember wishing at some point in the past that plot.*Plot.Draw were returning an error. so perhaps that's another weak data point for finally adding error to that signature ?)

[kortschak]

I'd be OK adding an error return, but I'd like to also do a visual panic on the plot as described above. This way if you ignore the error, you will see it in the output.

[kortschak]

Note that I recall Ethan being opposed to Draw having an error return, but I don't recall the reasoning.

[charlesdaniels]

if we just return c unmodified (in padY) under the elephant condition, we get the attached plot.

Maybe this is a silly question and I'm missing something obvious, but wouldn't it be possible to simply squish the box part in the vertical direction? As I understand it, the vertical size is always fixed, no? (of course this is all transposed for a vertical box plot).

[kortschak]

Is there reason to not just make the sizes correct in the first place?