plotter: add vector field plotter #543

kortschak · 2019-04-04T06:13:22Z

Please take a look.

codecov-io · 2019-04-04T06:19:58Z

Codecov Report

Merging #543 into master will increase coverage by 0.37%.
The diff coverage is 89.58%.

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   68.77%   69.15%   +0.37%     
==========================================
  Files          50       51       +1     
  Lines        5208     5304      +96     
==========================================
+ Hits         3582     3668      +86     
- Misses       1456     1464       +8     
- Partials      170      172       +2

Impacted Files	Coverage Δ
plotter/field.go	`89.58% <89.58%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd8a204...8ca2bf2. Read the comment docs.

codecov-io · 2019-04-04T06:19:58Z

Codecov Report

Merging #543 into master will increase coverage by 0.37%.
The diff coverage is 89.58%.

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   68.77%   69.15%   +0.37%     
==========================================
  Files          50       51       +1     
  Lines        5208     5304      +96     
==========================================
+ Hits         3582     3668      +86     
- Misses       1456     1464       +8     
- Partials      170      172       +2

Impacted Files	Coverage Δ
plotter/field.go	`89.58% <89.58%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd8a204...e4119b4. Read the comment docs.

ctessum · 2019-04-04T14:31:34Z

plotter/field.go

+	// Dims returns the dimensions of the grid.
+	Dims() (c, r int)
+
+	// Vector returns the value of a grid value at (c, r).


Maybe Vector returns the magnitude in the X and Y directions at grid column and row (c, r).

// Vector returns the value of a vector field at (c, r). // It will panic if c or r are out of bounds for the field.

field.go is derived heavily from heat.go and there are copy/paste errors that reflect that. This is one of those.

ctessum · 2019-04-04T14:32:34Z

plotter/field.go

+	// It will panic if c or r are out of bounds for the grid.
+	Vector(c, r int) XY
+
+	// X returns the coordinate for the column at the index x.


s/index x/index c/g

Done. This will also need to be fixed in heat.go.

ctessum · 2019-04-04T14:35:06Z

plotter/field.go

+}
+
+// NewField creates as new heat map plotter for the given data,
+// using the provided palette.


Provided palette?

ctessum · 2019-04-04T14:38:33Z

plotter/field.go

+	//
+	// If DrawGlyph is nil, a simple black arrow will
+	// be drawn.
+	DrawGlyph func(c vg.Canvas, v XY)


What if a user wants to change the color of the glyph based on the field magnitude? What if a user wants to scale the size of the glyph based on the square root of magnitude, or some other transform?

I think both of these things would be allowed if we made the field magnitude an argument here.

or maybe the normalized magnitude.

or, actually, providing the row and column indexes as arguments would probably be better, because then the user can pull in whatever external information they need.

If the user wants to change the colour based on the magnitude, they can calculate the magnitude from v. This was the major reason v was included (which is the normalised vector).

I don't like the idea of a completely free approach to transforms. It opens up whole new world of issues to have to answer. This is simple and works for the vast majority of cases with close to zero work for the user. Allowing scaling by other than linear seems to me like an anti-pattern; the two commonly used scalings are length and area (in that order). People are bad at assessing area compared to length (this is a reason why pie charts are bad).

I can see some merits in providing the row/column, but I need to think about it some more. I'm not sure the additional complexity wins much.

I somehow missed the fact that v was already included as an argument.

ctessum · 2019-04-04T15:46:20Z

plotter/field_test.go

@@ -0,0 +1,146 @@
+// Copyright ©2019 The Gonum Authors. All rights reserved.


To make the examples more legible in godoc, it might make sense to split the examples and tests up into individual files. From the documentation:

A whole file example is a file that ends in _test.go and contains exactly one example function, no test or benchmark functions, and at least one other package-level declaration. When displaying such examples godoc will show the entire file.

I think this is another PR. The example code that this is adapted from, heat_test.go is the same. In fact I think they all are because the TestX funcs are in the same files.

ctessum · 2019-04-04T15:46:56Z

plotter/field_test.go

+func (f field) Vector(c, r int) plotter.XY { return f.fn(f.X(c), f.Y(r)) }
+func (f field) X(c int) float64 {
+	if c < 0 || c >= f.c {
+		panic("index out of range")


"x index out of range"

This is example code, but OK.

Same in heat_test.go.

ctessum · 2019-04-04T15:47:11Z

plotter/field_test.go

+}
+func (f field) Y(r int) float64 {
+	if r < 0 || r >= f.r {
+		panic("index out of range")


"y index out of range"

ctessum · 2019-04-04T22:07:38Z

plotter/field.go

+	//
+	// If DrawGlyph is nil, a simple black arrow will
+	// be drawn.
+	DrawGlyph func(c vg.Canvas, v XY)


I somehow missed the fact that v was already included as an argument.

kortschak · 2019-04-04T22:20:00Z

I'll wait for @sbinet as well.

sbinet

LGTM (modulo 2 minor nit-picks)

plotter/field.go

kortschak · 2019-04-05T10:15:30Z

PTAL

kortschak · 2019-04-05T10:51:54Z

AppVeyor is horrifically slow here. Do we use your identity on that @sbinet? We probably shouldn't since you're doing other things as well. Do they have organisation accounts like travis does? I have to say, the service they have is pretty disappointing.

sbinet · 2019-04-05T11:46:42Z

their handling of org accounts is a bit confusing, and, at the very least not very straighforward, indeed...

kortschak requested review from ctessum and sbinet April 4, 2019 06:13

kortschak force-pushed the plotter/fields branch from e870227 to 8ca2bf2 Compare April 4, 2019 06:13

kortschak force-pushed the plotter/fields branch 4 times, most recently from 874ca5f to f7aefe5 Compare April 4, 2019 07:35

plotter: add vector field plotter

e4119b4

kortschak force-pushed the plotter/fields branch from f7aefe5 to e4119b4 Compare April 4, 2019 09:38

ctessum reviewed Apr 4, 2019

View reviewed changes

kortschak added 2 commits April 5, 2019 06:33

plotter: address pr comments

c7c0a3c

plotter: add line styling to Field

0735d75

ctessum previously approved these changes Apr 4, 2019

View reviewed changes

This was referenced Apr 5, 2019

plotter: fix documentation in HeatMap and examples #544

Closed

plotter: improve godoc formatting of examples #545

Closed

sbinet previously approved these changes Apr 5, 2019

View reviewed changes

plotter/field.go Show resolved Hide resolved

plotter/field.go Show resolved Hide resolved

plotter: address pr comments

8e747f1

kortschak dismissed stale reviews from sbinet and ctessum via 8e747f1 April 5, 2019 10:15

sbinet approved these changes Apr 5, 2019

View reviewed changes

kortschak merged commit f2023ba into master Apr 5, 2019

kortschak deleted the plotter/fields branch April 9, 2019 09:46

		@@ -0,0 +1,146 @@
		// Copyright ©2019 The Gonum Authors. All rights reserved.

plotter: add vector field plotter #543

plotter: add vector field plotter #543

Uh oh!

Conversation

kortschak commented Apr 4, 2019

Uh oh!

codecov-io commented Apr 4, 2019

Codecov Report

Uh oh!

codecov-io commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctessum Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kortschak Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kortschak commented Apr 4, 2019

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kortschak commented Apr 5, 2019

Uh oh!

kortschak commented Apr 5, 2019

Uh oh!

sbinet commented Apr 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Apr 4, 2019 •

edited

Loading

ctessum Apr 4, 2019 •

edited

Loading

kortschak Apr 4, 2019 •

edited

Loading