Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plotter: Implement XYers interface for polygons #555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

plotter: Implement XYers interface for polygons #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions plotter/plotter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,27 @@ type XYer interface {
XY(int) (x, y float64)
}

// XYers wraps a set of (x, y) pair sets.
type XYers interface {
// Len returns the number of XYers.
Len() int

// LenAt returns the length of XYer i.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to not define this as

type XYers interface {
    // Len returns the number of XYers.
    Len() int

    // XYer returns the XYer at position i.
    XYer(i int) XYer
}

This seems simpler for the user to create if needed.

Copy link
Contributor Author

@ctessum ctessum May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that if the interface returns plotter.XYer, then every type that implements it also needs to return plotter.XYer (returning a different interface with the same signature is not allowed). So every package that implements this interface would have to import plotter, and the same interface could never really be used for anything else.

This problem is avoided if the interface only includes built-in types.

For my specific use case, I have a general purpose geometry library and I'm trying to make it convenient to plot the polygons, but I don't really want to include plotter as a dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, OK. The API is not very nice like this though. I'll let someone else comment. @sbinet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a quick note to say I haven't forgotten about this.
I'll circle back to it before the end of the week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be more in favour of Dan's API as well.

I understand you don't want plotter as a dependency of github.com/ctessum/geom, and I agree it isn't great for the separation of concerns (or just the sheer amount of dependencies that come with gonum/plot.)
I also have had that concern and this kind of issue for my hep/hbook package that deals with histograms, but just the statistical aspects of histograms (1D, 2D, ...), not dealing with actually displaying them.

I didn't want hbook to be tied to a specific graphical library having been bitten by this kind of hard dependency in the (C++) particle physics world.
what I did was to create a separate package would depend on hep/hbook and gonum/plot to hold types that would implement the needed interfaces for gonum/plot to work, wrapping hep/hbook values. (it's hep/hplot: https://godoc.org/go-hep.org/x/hep/hplot)

wouldn't this kind of approach (the usual additional indirection switcheroo trick) work for you as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like that would result in a worse overall API experience, and I feel like there's a lot to be said for the interfaces only returning built-in types, but if that's the consensus, I'm alright with it I guess.

LenAt(i int) int

// XYer returns a set of x, y pairs.
XY(i, j int) (x, y float64)
}

// xyerAt implements the XYer interface for an item in an XYers.
type xyerAt struct {
XYers
i int
}

func (xy xyerAt) Len() int { return xy.XYers.LenAt(xy.i) }
func (xy xyerAt) XY(j int) (x, y float64) { return xy.XYers.XY(xy.i, j) }

// XYRange returns the minimum and maximum
// x and y values.
func XYRange(xys XYer) (xmin, xmax, ymin, ymax float64) {
Expand Down
8 changes: 4 additions & 4 deletions plotter/polygon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ type Polygon struct {
// differently, but all built-in backends treat inner rings
// with the opposite winding order from the outer ring as
// holes.
func NewPolygon(xys ...XYer) (*Polygon, error) {
data := make([]XYs, len(xys))
for i, d := range xys {
func NewPolygon(rings XYers) (*Polygon, error) {
data := make([]XYs, rings.Len())
for i := 0; i < rings.Len(); i++ {
var err error
data[i], err = CopyXYs(d)
data[i], err = CopyXYs(xyerAt{XYers: rings, i: i})
if err != nil {
return nil, err
}
Expand Down
18 changes: 14 additions & 4 deletions plotter/polygon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ import (
"gonum.org/v1/plot/vg/recorder"
)

// rings implements the XYers interface.
type rings []plotter.XYs

func (r rings) Len() int { return len(r) }
func (r rings) LenAt(i int) int { return len(r[i]) }
func (r rings) XY(i, j int) (x, y float64) {
p := r[i][j]
return p.X, p.Y
}

// ExamplePolygon_holes draws a polygon with holes, showing how
// the different built-in vg backends render polygons with holes.
// The output of this example is at
Expand All @@ -38,7 +48,7 @@ func ExamplePolygon_holes() {
// winding order as the outer polygon.
inner2 := plotter.XYs{{X: 3.5, Y: 2.5}, {X: 2.5, Y: 2.5}, {X: 2.5, Y: 3.5}, {X: 3.5, Y: 3.5}}

poly, err := plotter.NewPolygon(outer1, inner1, inner2)
poly, err := plotter.NewPolygon(rings{outer1, inner1, inner2})
if err != nil {
log.Panic(err)
}
Expand Down Expand Up @@ -94,13 +104,13 @@ func TestPolygon_holes(t *testing.T) {
// https://github.com/gonum/plot/blob/master/plotter/testdata/polygon_hexagons_golden.png.
func ExamplePolygon_hexagons() {
// hex returns a hexagon centered at (x,y) with radius r.
hex := func(x, y, r float64) plotter.XYs {
hex := func(x, y, r float64) rings {
g := make(plotter.XYs, 6)
for i := range g {
g[i].X = x + r*math.Cos(math.Pi/3*float64(i))
g[i].Y = y + r*math.Sin(math.Pi/3*float64(i))
}
return g
return rings{g}
}

p, err := plot.New()
Expand Down Expand Up @@ -166,7 +176,7 @@ func TestPolygon_hexagons(t *testing.T) {
// polygons wholly outside of the plotting range.
func TestPolygon_clip(t *testing.T) {
poly, err := plotter.NewPolygon(
plotter.XYs{{0, 0}, {1, 0}, {1, 1}, {0, 1}, {0, 0}},
rings{{{0, 0}, {1, 0}, {1, 1}, {0, 1}, {0, 0}}},
)
if err != nil {
t.Fatal(err)
Expand Down