plotter: use a fixed time zone for tests #278

sbinet · 2016-04-22T08:41:29Z

Fixes #276

kortschak · 2016-04-22T10:34:36Z

It's been 132 years. I think it's time to let go ;P

This does not fix the problem; UTC is just as fixed as Paris time. I think the issue is interplay with the locale that is used by formatting and the locale that is used during parsing, though I can't find a combinations of locales that fixes it.

eaburns · 2016-04-22T10:50:22Z

You need to use the local timezone to create the original time values. The timezone is lost when you convert to unix seconds;time.Unixalways assumes its input is in the local timezone. If you create the initial times in the local zone thentime.Unix` will be correct in its assumption that the times are local, and you will get the correct time back.

kortschak · 2016-04-22T10:56:25Z

I tried that when I reported the issue and it doesn't work:

$ git diff
diff --git a/plotter/timeseries_test.go b/plotter/timeseries_test.go
index 9954874..dbbfcf6 100644
--- a/plotter/timeseries_test.go
+++ b/plotter/timeseries_test.go
@@ -31,7 +31,7 @@ func Example_timeSeries() {
                )
                pts := make(XYs, n)
                for i := range pts {
-                       date := time.Date(2007+i, month, day, hour, min, sec, nsec, time.UTC).Unix()
+                       date := time.Date(2007+i, month, day, hour, min, sec, nsec, time.Local).Unix()
                        pts[i].X = float64(date)
                        pts[i].Y = float64(pts[i].X+10*rand.Float64()) * 1e-9
                }
$ go test
--- FAIL: TestImagePlot (0.02s)
    general_test.go:74: image mismatch for image_plot.png
    general_test.go:94: golden image has been written out as image_plot_golden.png
--- FAIL: TestTimeSeries (0.02s)
    general_test.go:74: image mismatch for timeseries.png
    general_test.go:94: golden image has been written out as timeseries_golden.png
FAIL

There is another failure there, which I have not yet investigated.

eaburns · 2016-04-22T13:41:15Z

Are those two separate thoughts: 1. that you tried it, and 2. that you
found another failure? Or is the second a consequence of the first?

Anyway, I'm still confused. How can it fail if you use local time? If you
create a date in your local timezone, convert it to unix seconds, convert
it back is it a different time? In other words, what does this program
print when you run it locally: http://play.golang.org/p/3AjCyKJsIX ? It
should print true. If so, then I don't see how doing the exact same thing
for the test would fail.

On Fri, Apr 22, 2016 at 6:56 AM Dan Kortschak notifications@github.com
wrote:

I tried that when I reported the issue and it doesn't work:

$ git diff
diff --git a/plotter/timeseries_test.go b/plotter/timeseries_test.go
index 9954874..dbbfcf6 100644
--- a/plotter/timeseries_test.go
+++ b/plotter/timeseries_test.go
@@ -31,7 +31,7 @@ func Example_timeSeries() {
)
pts := make(XYs, n)
for i := range pts {
                  date := time.Date(2007+i, month, day, hour, min, sec, nsec, time.UTC).Unix()
                  date := time.Date(2007+i, month, day, hour, min, sec, nsec, time.Local).Unix()
                pts[i].X = float64(date)
                pts[i].Y = float64(pts[i].X+10*rand.Float64()) \* 1e-9
        }
$ go test
--- FAIL: TestImagePlot (0.02s)
general_test.go:74: image mismatch for image_plot.png
general_test.go:94: golden image has been written out as image_plot_golden.png
--- FAIL: TestTimeSeries (0.02s)
general_test.go:74: image mismatch for timeseries.png
general_test.go:94: golden image has been written out as timeseries_golden.png
FAIL
There is another failure there, which I have not yet investigated.

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#278 (comment)

sbinet · 2016-04-22T20:53:03Z

so... the fix would actually be to modify:
https://github.com/gonum/plot/blob/master/axis.go#L483

to return a time.Time in UTC, right?

t := time.Unix(int64(tick.Value), 0).UTC()

then, perhaps the right fix would be to add another field to UnixTimeTicks:

type UnixTimeTicks struct {
    // Ticker is used to generate a set of ticks.
    // If nil, DefaultTicks will be used.
    Ticker Ticker

    // Format is the textual representation of the time value.
    // If empty, time.RFC3339 will be used
    Format string

    // Convert takes a float64 value and converts it into a time.Time.
    // If nil, time.Unix will be used.
    Convert func(float64) time.Time
}

kortschak · 2016-04-23T02:44:48Z

They are separate things. I didn't look into the second failure because the test golden image mutation gets in the way of debugging the issue.

eaburns · 2016-04-23T10:53:57Z

@sbinet, another option is instead of having Convert, have the user specify a timezone in the ticker, defaulting to Local. However, I still want to know why using Local all of the way through doesn't work.

kostya-sh · 2016-05-24T13:32:11Z

I have looked into this issue and found the following:

In order to produce a plot with the same date ticks as the golden image the same timestamps and location as were used to produce the golden image must be used. This means that timestamps should be created using UTC timezone but formatted using (I assume) Europe/Paris.
I also discovered that when I run tests locally (using the latest versions of all libraries and Go 1.7 from tip) all tests comparing images against the golden files fail. Visually test images look the same though.

I hope this helps.

As far as the code change is concerned I would prefer to have UnixTimeTicks.Location field instead of UnixTimeTicks.Convert function (as suggested by @eaburns).

kostya-sh · 2016-05-24T13:55:37Z

With Go 1.6.2 all tests pass (apart from timestamp test as I am in UTC+8). I suspect that Go 1.7 produces different images comparing to Go 1.6 due to compression algorithms changes in compress/flate package.

sbinet · 2016-05-24T15:58:37Z

@kostya-sh thanks for giving it a try (and raising the issue wrt tip.)

wrt having a Convert field or a Location field:
having a Convert func field allows the user to have complete control on how the 64bits of a time.Time are to be encoded and then interpreted. (perhaps you want UnixNano() instead of Unix(), perhaps you want UTC(), etc...)
You could imagine that instead of simply converting the returned int64 value into a float64 one, you may have a function that retrieves the int64 bits payload and encodes it directly (via math.Float64frombits) into a int64. No rounding issue for big int64 values.

Just for my own information, did my alternate PR (the #278 one with Convert) fix the UTC problem?

kostya-sh · 2016-05-24T23:56:35Z

@sbinet,

Convert vs Location. There is only one way to convert unix time (number of seconds since epoch) and timezone (time.Location) to time.Time object. Any other conversion is no longer a unix time conversion. If more flexibility is required then maybe UnixTimeTicks should be renamed to TimeTicks and a few standard conversion functions are provided. E.g.:

type TimeTicks {
    ...
    Conversion: TimeConversion
}

type TimeConversion func(float64) time.Time

func UnixTime(loc *time.Location) TimeConversion {
    ....
}

?

Your PR with UTC fixes the problem, but if you use Local() both to create timestamps and to convert them then the test fails because the golden file has been created using different data. To fix it you can either:

regenerate the golden file (using Local() conversion)
Use the timezone that was used to create golden file in your example (return time.Unix(int64(v), 0).In(time.FixedZone("Europe/Paris", 120)))

kortschak · 2016-05-25T00:18:03Z

If more flexibility is required then maybe UnixTimeTicks should be renamed to TimeTicks and a few standard conversion functions are provided.

This sounds good to me.

This CL introduces the TimeFloatConverter interface that describes how a time.Time value is converted to a float64 (and back.) A default TimeFloatConverter (used by TimeTicks) is also provided in the shape of UnixTimeConverter. Test updated to be resilient against timezones (tested with 'Europe/Paris', 'America/Los_Angeles' and 'Australia/Sydney') Fixes gonum#278.

sbinet · 2016-05-25T10:05:30Z

PTAL.

sbinet · 2016-05-25T10:14:22Z

interestingly, my first approach of (un)marshaling time.Time values into float64 via math.Float64frombits didn't work because (presumably, haven't checked) the Ticker interface expects a linear space to find ticks and adjacent time.Time values don't result in adjacent float64 values (generated via math.Float64frombits)

too bad.

kostya-sh · 2016-05-25T11:14:56Z

@sbinet I think converting from time.Time to float64 (TimeFloatConverter.MarshalTime) should not be part of this package because:

this functionality is not used in the pacakge itself
it is cumbersome to use this functionality from outside of the package (t.Unix() is much simler than creating plot.UnixTimeConverter{} and calling converter.MarshalTime(t)). Ideally I want the time formatting to work reasonably just by setting Tick.Marker = plot.TimeTicks{}.

Secondly it is not possible to customize Location that is used to format dates. Currenly it is hardcoded to UTC.

See also some comments inline.

kostya-sh · 2016-05-25T11:16:32Z

axis.go

-// UnixTimeTicks expects values in Unix time seconds.
-type UnixTimeTicks struct {
+// TimeTicks is suitable for axes representing time values.
+// By default, TimeTicks expects values in (UTC) Unix time seconds.


Unix time represents moment in time. It is time zone independent, so (UTC) Unix time seconds doesn't make sense.

kostya-sh · 2016-05-25T11:32:31Z

I left a few comments however despite being the one who proposed the idea of having more flexible TimeTicks type I really like the simplicity of the original UnixTimeTicks type and still think that adding Location field to it was all that required ;)

sbinet · 2016-05-25T11:35:26Z

@kostya-sh , yes, it is somewhat a bit less compact to write.
on the plus side, the protocol to encode/decode time.Time values to/from float64 ones is, IMHO, clearer, with less opportunities to introduce impedance mismatch or incorrect expectations.

I could introduce:

func UnixTimeTicks(format string, cnv TimeFloatConverter) TimeTicks {
    if format == "" {
        format = time.RFC3339
    }
    if cnv == nil {
        cnv = UnixTimeConverter{}
    }
    return TimeTicks{
        Format: format,
        Converter: cnv,
    }
}

to have a more direct way to have a properly configured TimeTicks

Fixes gonum#276

This CL introduces the TimeFloatConverter interface that describes how a time.Time value is converted to a float64 (and back.) A default TimeFloatConverter (used by TimeTicks) is also provided in the shape of UnixTimeConverter. Test updated to be resilient against timezones (tested with 'Europe/Paris', 'America/Los_Angeles' and 'Australia/Sydney') Fixes gonum#276.

kostya-sh · 2016-05-31T13:19:44Z

I had another look at ticker related functionality in plot package and I think it might be good idea to split Ticker interface into two: Ticker and Labeler.

type Ticker interface {
  func Ticks(min, max float64) []float64
}

type Labeler interface {
  func Label(tick float64) string
}

This will allow to specify ticker rules and formatting rules independently.

For example LogTicks and DefaultTicks and ConstantTicks use the same formatting rules but different ticker rules. UnixTimeTicks and DefaultTicks have the same ticker rules but different formatting rules.

Also if ConstantTicks is just a []float64 it is much easier to create in code.

It might be good idea in addition to the time specific formatter provide time specific ticker as well (that can prefer ticks at the beginning of major time intervals such as minutes or months depending on range).

Having a seprate Labeler interface will also make it easy to provide FloatLabeler with configurable precision if necessary.

Thoughts? Should I bring this question on the mainling list instead?

sbinet · 2016-05-31T16:14:10Z

SGTM.
I suppose sending a PR wouldn't hurt to see how that would look like.
(small(er) interfaces are good, anyways)

kostya-sh · 2016-05-31T23:25:41Z

Ok, I will send a PR for review soon.

kostya-sh · 2016-06-01T14:23:17Z

See #286

kortschak · 2016-07-12T07:29:03Z

Can we merge this in the interim. Having a failing case here is hampering what I can do.

kortschak · 2016-08-29T09:55:18Z

ping

kortschak · 2016-09-12T23:45:18Z

@sbinet Is there a particular hold up here that I can help with?

kortschak · 2016-10-24T10:38:40Z

I'm going to close this. If it is going to be revived, please reopen.

eaburns · 2016-10-24T16:05:29Z

I don't understand that logic. It certainly will not get reviewed if it's closed. Are you waiting for me or someone in particular to review?

kortschak · 2016-10-25T19:02:35Z

It's stale and hasn't moved for months. If I get some time I will try to put together a good.

sbinet force-pushed the time-loc branch from 87b498d to 0be9316 Compare April 22, 2016 21:11

sbinet mentioned this pull request May 24, 2016

travis: enable tests for go-1.7-beta #279

Closed

sbinet force-pushed the time-loc branch 2 times, most recently from 8081980 to fd4b9e8 Compare May 25, 2016 10:00

sbinet force-pushed the time-loc branch from fd4b9e8 to 24ef14b Compare May 25, 2016 10:03

kostya-sh reviewed May 25, 2016
View reviewed changes

sbinet and others added 2 commits May 28, 2016 17:21

plotter: use a fixed time zone for tests

a555d4f

Fixes gonum#276

axis: make float64-to-time conversion customizable

a26d2d9

sbinet force-pushed the time-loc branch from 24ef14b to a4fa625 Compare May 28, 2016 15:24

sbinet mentioned this pull request Jun 3, 2016

plot: split Ticker into Ticker and Labeler #286

Closed

kortschak mentioned this pull request Jul 15, 2016

Default labeling for X axis in timeseries is sparse. #296

Open

kortschak closed this Oct 24, 2016

kortschak mentioned this pull request Oct 25, 2016

plot,plotter: fix time tick location handling #317

Merged

plotter: use a fixed time zone for tests #278

plotter: use a fixed time zone for tests #278

Uh oh!

Conversation

sbinet commented Apr 22, 2016

Uh oh!

kortschak commented Apr 22, 2016

Uh oh!

eaburns commented Apr 22, 2016

Uh oh!

kortschak commented Apr 22, 2016

Uh oh!

eaburns commented Apr 22, 2016

Uh oh!

sbinet commented Apr 22, 2016

Uh oh!

kortschak commented Apr 23, 2016 via email

Uh oh!

eaburns commented Apr 23, 2016

Uh oh!

kostya-sh commented May 24, 2016

Uh oh!

kostya-sh commented May 24, 2016

Uh oh!

sbinet commented May 24, 2016

Uh oh!

kostya-sh commented May 24, 2016

Uh oh!

kortschak commented May 25, 2016

Uh oh!

sbinet commented May 25, 2016

Uh oh!

sbinet commented May 25, 2016

Uh oh!

kostya-sh commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostya-sh May 25, 2016

Choose a reason for hiding this comment

Uh oh!

kostya-sh commented May 25, 2016

Uh oh!

sbinet commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kostya-sh commented May 31, 2016

Uh oh!

sbinet commented May 31, 2016

Uh oh!

kostya-sh commented May 31, 2016

Uh oh!

kostya-sh commented Jun 1, 2016

Uh oh!

kortschak commented Jul 12, 2016

Uh oh!

kortschak commented Aug 29, 2016

Uh oh!

kortschak commented Sep 12, 2016

Uh oh!

kortschak commented Oct 24, 2016

Uh oh!

eaburns commented Oct 24, 2016

Uh oh!

kortschak commented Oct 25, 2016 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kostya-sh commented May 25, 2016 •

edited

Loading

sbinet commented May 25, 2016 •

edited

Loading