-
Notifications
You must be signed in to change notification settings - Fork 204
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.
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
Adds capability for colored scatter plots (issue #195) #396
Conversation
@ctessum Chris, I am a bit confused. If we decide not to merge this PR, and follow the directions from your comments here, may I ask 2 things that I do not clearly understand:
|
Hi @Scorpiokat, In Go, functions can be variables. How the concept I proposed would work is that we would add a field to the Because it is a field and not a method, by default There is already something similar implemented in the So, to answer your specific questions:
Let me know whether that makes sense. |
You may also like to see how bíogo rings does this kind of thing where data series are reflected on to see if they want to do styling. Note that, from memory, @eaburns has indicated that he does not really want to see things like this in the plotters here. |
@ctessum thanks you for your comments. I got the idea, but still not completely clear on the Z (or whichever variable that is), and how all this And will |
What I didn't include in the original explanation was that I was asssuming that |
An independent question is whether there is consensus regarding whether this is a good idea. This may not be the correct location for that discussion, however. |
@ctessum I have sent a new commit with the changes you have proposed. As this is a 2-in-1 PR, it became a bit messy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a first pass.
I would remove the scatterDataNew
data set from the ExampleScatter
as it clutters the output plot somewhat.
I would also completely remove the ScatterColor
type which isn't needed anymore.
plotter/scatter_test.go
Outdated
@@ -10,10 +10,14 @@ import ( | |||
"math/rand" | |||
"testing" | |||
|
|||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put "fmt"
and "os"
as part of the stdlib import block
plotter/scatter_test.go
Outdated
dc := draw.New(img) | ||
dc = draw.Crop(dc, 0, -legendWidth, 0, 0) // Make space for the legend. | ||
p.Draw(dc) | ||
w, err := os.Create("testdata/scatter.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are missing a defer w.Close()
here as well as an explicit err = w.Close()
(with the according error check) after the call to png.WriteTo(w)
.
plotter/scattercolor_test.go
Outdated
package plotter | ||
|
||
import ( | ||
"gonum.org/v1/plot/palette" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this gonum.org/v1/plot/palette
import should be part of the gonum.org
import block below.
plotter/scattercolor_test.go
Outdated
"math/rand" | ||
"testing" | ||
|
||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and fmt
and os
should be part of the stdlib import block above.
plotter/scattercolor_test.go
Outdated
dc := draw.New(img) | ||
dc = draw.Crop(dc, 0, -legendWidth, 0, 0) // Make space for the legend. | ||
p.Draw(dc) | ||
w, err := os.Create("testdata/scattercolor.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comments than above about defer w.Close()
and the explicit w.Close()
.
plotter/scattercolor_test.go
Outdated
"os" | ||
) | ||
|
||
// ExampleScatter draws some scatter coloured points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ExampleScatter/ExampleScatter_color/
plotter/scattercolor_test.go
Outdated
) | ||
|
||
// ExampleScatter draws some scatter coloured points. | ||
func ExampleScatterColor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ExampleScatter/ExampleScatter_color/
plotter/scattercolor_test.go
Outdated
|
||
pal := palette.Heat(12, 1) | ||
|
||
sc, err := NewScatterColor(scatterColorData, pal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use NewScatter
instead.
plotter/scattercolor.go
Outdated
// determines the colour of a scatter plot point. | ||
// Scatter implements the Plotter interface, drawing | ||
// a glyph for each of a set of points. | ||
type ScatterColor struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this type. it's not needed anymore.
Thank you @sbinet . I decided to delete I am not very happy about the look of the legend on the right though, but have no idea on a better way to locate it. |
I personnally prefered keeping the original scatter example untouched and add a new one exercizing the new scatter-color plot. (but I'll let others chime in) |
plotter/scatter.go
Outdated
|
||
s.GlyphStyle = DefaultGlyphStyle | ||
|
||
s.GlyphStyleFunc = func(int) draw.GlyphStyle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could keep it nil
, couldn't we? (as we handle the nil
case in the Scatter.Plot
method)
actually, I think we could leave the whole NewScatter
function untouched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comment above is still relevant, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I make it nil
, I got a warning about a unability to use it in this case.
And a lot of tests (TestErrors, TestMainExample, TestTimeSeries, for instance) use DeafultGlyphStyle
, so a lot of tests fail in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about GlyphStyleFunc
being nil
. not GlyphStyle
(which is indeed set by default to DefaultGlyphStyle
.)
I believe that, if you leave the whole NewScatter
unmodified with regard to what it was before, then everything should work well.
ie: NewScatter
should look like:
// NewScatter returns a Scatter that uses the
// default glyph style.
func NewScatter(xys XYer) (*Scatter, error) {
data, err := CopyXYs(xys)
if err != nil {
return nil, err
}
return &Scatter{
XYs: data,
GlyphStyle: DefaultGlyphStyle,
}, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, sorry. Did not understand that. Now it works with the original NewScatter
.
plotter/scatter.go
Outdated
for _, p := range pts.XYs { | ||
c.DrawGlyph(pts.GlyphStyle, vg.Point{X: trX(p.X), Y: trY(p.Y)}) | ||
for i, p := range pts.XYs { | ||
if pts.GlyphStyleFunc != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of testing over and over whether GlyphStyleFunc
is nil or not, couldn't we create a little closure (before the for-loop) that will return the correct GlyphStyle
value?
something like:
glyph := func(i int) draw.GlyphStyle { return pts.GlyphStyle }
if pts.GlyphStyleFunc != nil {
glyph = pts.GlyphStyleFunc
}
for i, p := range pts.XYs {
c.DrawGlyph(glyph(i), vg.Point{X: trX(p.X), Y: trY(p.Y)})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do absolutely agree with you, have made the changes.
As for a separate color scatter plot, I am happy to bring it back, as it looks much better on its own, than together with 3 others. |
yes, I think having a separate example (and thus a separate test) would be better. |
Looks better now? |
yes. |
plotter/scatter.go
Outdated
@@ -16,6 +16,10 @@ type Scatter struct { | |||
// XYs is a copy of the points for this scatter. | |||
XYs | |||
|
|||
//GlyphStyleFunc, if not nil, specifies GlyphStyles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please leave a space between //
and the start of the comment.
plotter/scatter.go
Outdated
@@ -16,6 +16,10 @@ type Scatter struct { | |||
// XYs is a copy of the points for this scatter. | |||
XYs | |||
|
|||
//GlyphStyleFunc, if not nil, specifies GlyphStyles | |||
//for individual points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well.
plotter/scatter.go
Outdated
|
||
s.GlyphStyle = DefaultGlyphStyle | ||
|
||
s.GlyphStyleFunc = func(int) draw.GlyphStyle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comment above is still relevant, I believe.
plotter/scatterColor_test.go
Outdated
"gonum.org/v1/plot/vg/vgimg" | ||
) | ||
|
||
// ExampleScatterColor draws some scatter points, a line, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename this as: ExampleScatter_color
so it looks well on godoc (and is part of the examples for Scatter
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still there :)
also, the comment doesn't match what the example actually does :)
perhaps something along these lines would do ?
// ExampleScatter_color draws some scatter points.
// Each point will be plotted with a different color depending on some external criteria.
func ExampleScatter_color() {
(it could be improved, though.)
plotter/scatterColor_test.go
Outdated
|
||
// ExampleScatterColor draws some scatter points, a line, | ||
// and a line with points. | ||
func ExampleScatterColor() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename this as: ExampleScatter_color
so it looks well on godoc (and is part of the examples for Scatter
.)
plotter/scatterColor_test.go
Outdated
} | ||
|
||
p.Add(sc) | ||
//p.Legend.Add("",sc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that comment could be removed, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far!
plotter/scatterColor_test.go
Outdated
|
||
// randomPoints returns some random x, y points | ||
// with some interesting kind of trend. | ||
randomPoints := func(n int) XYs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for this function to return XYZs
instead of XYs
? Then we wouldn't need to separately create z values below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we will have to include Z
to the type Scatter
, won't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change it to be the same randomTriples
function that is in the bubbles example.
plotter/scatterColor_test.go
Outdated
|
||
z := []float64{31, 41, 51, 61, 71, 81, 91, 101, 111, 121, 131, 141, 151, 161, 171, 181} | ||
|
||
sc.GlyphStyleFunc = func(i int) draw.GlyphStyle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment describing what this is doing?
plotter/scatterColor_test.go
Outdated
|
||
p.Add(sc) | ||
|
||
// Create a legend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a better way to create the legend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for example:
pLegend, err := plot.New()
if err != nil {
log.Panic(err)
}
l := &ColorBar{ColorMap: colors}
l.Vertical = true
pLegend.Add(l)
pLegend.HideX()
pLegend.Y.Padding = 0
pLegend.Title.Text = "Title"
Then, below:
dcPlot := draw.Crop(dc, 0,-legendWidth, 0, 0)
dcLegend := draw.Crop(dc, 300-legendWidth, 0, 0, 0)
pLegend.Draw(dcLegend)
p.Draw(dcPlot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution is a bit confusing, as it adds the legend (and the values which our z[i]
is taking) on the Y axis (or X axis in case of the l.Horizontal
option). So,I kept the previous solution for now.
One additional outstanding issue is that since the One way to test that this is implemented correctly would be to add a |
@ctessum this is the same solution, I have just added some stats to it in the last commit to show that the system builds a file that differs in size from the golden one. However, on the systems I have tested the code, these 2 files are identical, so the test does not fail. I will remove these lines once we find out why that happens. |
If you do go get -u ./... on your system to update all the dependencies,
does the test still pass?
…On Sat, Nov 18, 2017, 4:38 PM Ekaterina Efimova ***@***.***> wrote:
@ctessum <https://github.com/ctessum> this is the same solution, I have
just added some stats to it in the last commit to show that the system
builds a file that differs in size from the golden one. However, on the
systems I have tested the code, these 2 files are identical, so the test
does not fail. I will remove these lines once we find out why that happens.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#396 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AF6AAYuCNafUrTE61ACM9OQ_oLWtYG4Oks5s3pd7gaJpZM4PxXs3>
.
|
@ctessum yes. On this machine, and on a new freshly made one. |
I get perfect a perfect match on my machine (test passes and sha1 matches). A suggestion I have is to render the png to base64 (we have a helper for this). Then at least we can see what the image looks like. Additionally, you can make changed in cmpimg to see in detail why it's failing the check. |
@kortschak and what sha1 are you getting on your machine? Could you please send that to me? And that would be really helpful if I could have got the image that the system is generating, otherwise I cannot test it against my result. just by making commits. |
The sha1 I get is f0217b5efc20f13c2c43a4e5a760b23eef9d1eec. What you can do is use encoding/base64 to output a text representation of the file to the travis log. Then you can reconvert that to a binary after scraping it from the log. |
…to the terminal in travis
@kortschak I have done that. The base64 representations are different. However, when reconverted to binary - they look the same, while sha1sum says otherwise. Looks like cmpimg needs to be inspected too. |
I have compared the two images and they are nearly the same, but there is a very slight difference in the colour spectrum at the bottom of the plot (not visible with a straight subtraction of images, but very clear when the intensity difference is scaled by ~25x). What we can see from this is that the failure is entirely from the colourbar (this is easy to check by removing that code and seeing if the failure persists). What I suspect is the underlying cause is floating point differences between the arch we are running and the arch travis is using. We've seen things like this before in Gonum proper (lapack being a good example). I don't know why it's affecting the bar and not the scatter dots, but I suspect it is the bar image scaling. If this is the problem, the obvious solution is to remove the colorbar. |
@kortschak thanks a lot for checking the images! I haven't noticed that difference in colour. Changing ColorBar back to PaletteThambnailers definitely worked for the best now. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please wait for @ctessum.
LGTM, but remember to squash all the commits into one, or perhaps two commits: one for everything except for the last commit here, and then a second one where the colorbar is changed to the palette thumbnails. |
It's also worth rebasing onto master (there are conflicts that will make this difficult though) since default ticks behaviour has changed. I'd rebase -i into the structure that you want, then attempt the rebase onto master and fix any conflicts that arise. |
There's still a lot going on in these commits that is not relevant to the actual change (left over from the debugging). To avoid a lot of messing around, I think we should just merge this into one commit. However, it still needs to be rebased onto master; the failure you see is due to a disagreement between the old and the new default ticks. So, can you try to rebase onto the gonum plot master and then regen the golden image and push it back here. |
Dan @kortschak I have regenerated files, but the one that got regenerated is just one which now is conflicting. ScatterColor_golden has passed the tests on my machine, i.e. was not regenerated. I guess, the Travis check will be failing again once the existing conflict is resolved. |
Travis is correct here, the current gonum/plot master is not in the history of this PR. To get this, you need to rebase onto gonum/plot's master; your master needs to be made to match the gonum/plot master. Unfortunately you have squashed the two commits together prior to thie merge of your master (I must have been unclear - we squash when we merge). I've gone through the reflog to find that.
check everything is OK
|
Thank you for such a detailed reply. However, not everything is still OK:
|
Oh, sorry, that requires, auth use |
Looks like it has finally passed the checks. |
We decided to leave the ColorBar issue for later, right? Or...? |
You mean the float errors? |
We have 25 commits back, sorry. I am just going to leave this and squash them in the merge. |
Yes, I meant that. |
Without knowing what is going on on their hardware, there is not much we can do. |
Sure. |
I should have checked. That was not rebased onto the gonum/plot master and was passing because the old default ticks algorithm was still being used. I have sent a PR fixing the breakage. |
Sorry and thanks a lot. |
I have closed the previous pull request, and created this one according to your comments @kortschak and @ctessum. Thanks!
@ctessum I decided to send this PR anyway (and it's up to you, obviously, to accept it or not right now), so that at least the plot has this capability and it will fix the #195 issue. Meanwhile, I will start working on the alternative solution which you have proposed. It should definitely look much more elegant and be an improvement of the existing way of making scatter, scatter colour and bubble plots.