Time scale improvements #3914

tredston · 2017-02-15T18:32:30Z

Complete work started in https://github.com/chartjs/Chart.js/tree/time-scale-improvements (#3673) to convert scale.time to internally use unix timestamps instead of moments. Improves performance and reduces complexity.

etimberg · 2017-02-15T22:37:52Z

Thanks @tredston I will look at reviewing this. In the meantime, could you rebase against master and squash into some smaller commits?

etimberg

Overall this is a great start. Thank you for getting this to PR quality 😄

I had a few comments, questions, etc and some minor refactoring that I suggested in the comments. Please take a look and we can discuss if you have questions or concerns

etimberg · 2017-02-15T22:49:05Z

src/scales/scale.time.js

+		var units = Object.keys(time);
+		var unit;
+
+		for (var i = units.indexOf(minUnit); i < units.length; i++) {


would a while loop be slightly more clear here?

while (i < units.length && !fits)

or even maybe a do-while since we always want to loop to run at least once and we can make it clear.

do { } while (i < units.length && !fits);

I prefer to use a for loop when doing simple array iteration, I think having the start index, end conditions and stride all together is clearer than spreading them out.

The fits variable would also need to be hoisted out of the loop, in which case we could write:

for (var i = units.indexOf(minUnit); i < units.length && !fits; i++)

instead?

i think we could leave this as is. I had just wondered if we could make it a bit simpler but it's already straightforward

etimberg · 2017-02-15T22:51:01Z

src/scales/scale.time.js

-			name: 'month',
+		},
+		month: {
+			size: 2.628e9,


One question that came up from when I first worked on this ... what do we do for months where this is not true? ie, months like February that have less days.

These sizes are used to estimate the unit and step size to use, they aren't used for calculating the tick locations. The step size is a multiple of the unit, so a unit of month and stepSize of 3 give ticks which are 3 months apart. The labels are still rounded (by moment) to the start of the unit so they display correctly.

See e5cd916 for details.

Ah, ok, great 👍

etimberg · 2017-02-15T23:24:41Z

src/scales/scale.time.js

+		var stepSize = generationOptions.stepSize;
+		ticks.push(generationOptions.min !== undefined ? generationOptions.min : niceRange.min);
+		var cur = moment(niceRange.min);
+		while (cur.add(stepSize, generationOptions.unit).valueOf() < niceRange.max) {


Does this still need to use moment? I thought once everything was timestamps this wasn't needed? Or does this make things a lot simpler overall?

This simplifies units which don't have a constant number of milliseconds in them eg months, years.

ok, makes sense 😄

etimberg · 2017-02-15T23:26:34Z

src/scales/scale.time.js

+		var niceMin;
+		var niceMax;
+		var isoWeekday = generationOptions.isoWeekday;
+		if (isoWeekday !== false) {


I think this option only applies if generationOptions.unit is 'week' otherwise it could be transforming data that is in seconds, or minutes.

Yep, will update to only apply when generationOptions.unit is week.

etimberg · 2017-02-15T23:47:23Z

src/scales/scale.time.js

+		getLabelCapacity: function(exampleTime) {
+			var me = this;
+
+			me.displayFormat = me.options.time.displayFormats.millisecond;	// Pick the longest format for guestimation


would be nice if we didn't have to set this on ourself .. could we refactor to pass this into tickFormatFunction instead?

The problem is that convertTicksToLabels uses tickFormatFunction as the callback for a map so we can't really change tickFormatFunction's message signature to include the display format.

etimberg · 2017-02-15T23:48:49Z

test/scale.time.tests.js

-		// scale.buildTicks();
-		scale.update(400, 50);
+		var scale = createScale(mockData, config);
+		scale.update(5000, 200);


does this need to be this wide?

It just needed to be wide enough to not be in danger of having ticks dropped for overlapping. Looking again this can be revised down to about 2500 before the last two ticks might overlap.

Alternatively we can have a narrower graph and update the expect to remove the penultimate tick.

etimberg · 2017-02-15T23:49:06Z

test/scale.time.tests.js

-		// scale.buildTicks();
-		scale.update(400, 50);
+		var scale = createScale(mockData, config);
+		scale.update(800, 200);


same comment here about the width

etimberg · 2017-02-15T23:50:34Z

test/scale.time.tests.js

 	});

-	it('Should use the isoWeekday option', function() {
-		var scaleID = 'myScale';
+	it('should not have overlapping ticks', function() {


Any reason there are two tests with the same name?

One (in describe('when specifying limits'... suite) covers the case when specifying a time.max while the other covers the general case. Since they are in separate suites jasmine will print different messages if/when they break.

ok, just wanted to make sure this wasn't an error 😄

etimberg · 2017-02-15T23:51:07Z

test/scale.time.tests.js

+			}
+		});
+		var scale = chart.scales.xScale0;
+		scale.update(24633, 160);


this seems like a giant width :/

This is essentially a regression test, the code was lifted from the example in #2249.

ah, ok. that makes sense

etimberg · 2017-02-15T23:52:30Z

test/scale.time.tests.js

-		expect(xScale.getPixelForValue('', 0, 0)).toBeCloseToPixel(71);
-		expect(xScale.getPixelForValue('', 6, 0)).toBeCloseToPixel(452);
-		expect(xScale.getPixelForValue('2015-01-01T20:00:00')).toBeCloseToPixel(71);
+		it('should be bounded by midnights', function() {


[nit] Is the goal of this test to ensure that that when days are displayed, the last tick is at the end of the last day and similarly for the first tick?

After reading the several years ones, I think a better title might be "should be bounded by the nearest day beginnings" since I was a little unclear on what this meant at first.

Yes that's the goal.

That title is much clearer, I will update.

etimberg · 2017-02-16T01:27:43Z

Tried these out and they work well. It seems really fast now 😄 I did notice that the add buttons on https://github.com/chartjs/Chart.js/blob/master/samples/scales/time/line-time-point-data.html don't work. This was likely broken when I started on this not by your changes afterwards. Do you mind fixing it while you're at it?

tredston · 2017-02-17T00:24:57Z

Squashed to fewer commits and rebased onto master.

Updated the code following discussions thus far.

Updated the sample as requested.

etimberg

Thanks @tredston

simonbrunel

Thanks @tredston :)

I'm not familiar with the time scale, so can't really review the overall logic, so just reviewed the code style.

That's a big PR, I didn't go in details so to sum up:

gather local variables in function header (one per line, except for uninitialized variables)
cache object property and array value when used more than 2 or 3 times
cache for loop end condition when object property or array value
use shorter names when context is obvious

I didn't review unit test!

simonbrunel · 2017-02-21T09:02:07Z

samples/scales/time/line-time-point-data.html

@@ -119,7 +119,7 @@

 		document.getElementById('addData').addEventListener('click', function() {
 			if (config.data.datasets.length > 0) {
-				var lastTime = myLine.scales['x-axis-0'].labelMoments[0].length ? myLine.scales['x-axis-0'].labelMoments[0][myLine.scales['x-axis-0'].labelMoments[0].length - 1] : moment();
+				var lastTime = myLine.scales['x-axis-0'].ticksAsTimestamps.length ? moment(myLine.scales['x-axis-0'].ticksAsTimestamps[myLine.scales['x-axis-0'].ticksAsTimestamps.length - 1]) : moment();


Could be more readable by caching myLine.scales['x-axis-0'].ticksAsTimestamps

var timestamps = myLine.scales['x-axis-0'].ticksAsTimestamps; var lastTime = timestamps.length? moment(timestamps[timestamps.length - 1]) : moment();

simonbrunel · 2017-02-21T09:04:17Z

src/scales/scale.time.js

@@ -61,253 +69,288 @@ module.exports = function(Chart) {
 				month: 'MMM YYYY', // Sept 2015
 				quarter: '[Q]Q - YYYY', // Q3
 				year: 'YYYY' // 2015
-			}
+			},


Why here and not at line 46?

Because there were multiple authors and linting (comma-dangle) allows it :)

Of course, I mean you modified both lines, but only added comma on the second one. Anyway, that's not important!

simonbrunel · 2017-02-21T09:06:00Z

src/scales/scale.time.js

+			// Custom parsing (return an instance of moment)
+			console.warn('options.time.format is deprecated and replaced by options.time.parser. See http://nnnick.github.io/Chart.js/docs-v2/#scales-time-scale');
+			return timeOpts.format(label);
+		}


No need for all these else since you return for all if:

if (foo) { return 'foo'; } if (bar) { return 'bar'; } return false;

simonbrunel · 2017-02-21T09:08:17Z

src/scales/scale.time.js

+		} else if (label.isValid && label.isValid()) {
+			// Moment support
+			return label;
+		} else if (typeof timeOpts.format !== 'string' && timeOpts.format.call) {


Can you cache timeOpts.format?

var format = timeOpts.format; if (typeof format !== 'string' && format.call) { return format(label); } return moment(label, format);

simonbrunel · 2017-02-21T09:09:39Z

src/scales/scale.time.js

+			return label;
+		} else if (typeof timeOpts.format !== 'string' && timeOpts.format.call) {
+			// Custom parsing (return an instance of moment)
+			console.warn('options.time.format is deprecated and replaced by options.time.parser. See http://nnnick.github.io/Chart.js/docs-v2/#scales-time-scale');


We try to reduce library size, so I would avoid to output the URL (which is quite outdated)

simonbrunel · 2017-02-21T09:45:15Z

src/scales/scale.time.js

+	 * @return {Number[]} ticks
+	 */
+	Chart.Ticks.generators.time = function(generationOptions, dataRange) {
+		var niceMin;


var isoWeekday = generationOptions.isoWeekday; var niceMin, niceMax; //< uninitialized variables on the same line

and generationOptions -> options

simonbrunel · 2017-02-21T09:50:08Z

src/scales/scale.time.js

 			}
+			var timeGeneratorOptions = {


No need for this temporary variable:

var ticks = me.ticks = Chart.Ticks.generators.time({ maxTicks: maxTicks, min: minTimestamp, max: maxTimestamp, stepSize: stepSize, unit: unit, isoWeekday: timeOpts.isoWeekday }, { min: dataMin, max: dataMax });

simonbrunel · 2017-02-21T09:51:06Z

src/scales/scale.time.js

-			return moment(label, me.options.time.format);
+
+			var tickLabelWidth = me.ctx.measureText(label).width;
+			var cosRotation = Math.cos(helpers.toRadians(me.options.ticks.maxRotation));


Can you cache var ticksOptions = me.options.ticks?

simonbrunel · 2017-02-21T10:03:30Z

src/scales/scale.time.js

@@ -8,34 +8,42 @@ module.exports = function(Chart) {

 	var helpers = Chart.helpers;
 	var time = {


This variable name could be a bit more explicit, time is quite a common term in this file :)

simonbrunel · 2017-02-21T12:19:39Z

src/scales/scale.time.js

+			var me = this;
+			var epochWidth = me.max - me.min;
+			var decimal = 0;
+			if (epochWidth > 0) {


var decimal = epochWidth? (offset - me.min) / epochWidth : 0;

tredston · 2017-02-27T13:36:10Z

Thanks for the review @simonbrunel.

I've implemented the requested changes around caching local variables and logic simplification.

However, I'm not keen to implement:

gather local variables in function header (one per line, except for uninitialized variables)

beacuse:

there are linting rules for this (one-var and vars-on-top) which are explicitly disabled.
splitting up declaration and assignment makes the code more difficult to follow, as well as more difficult to migrate to using const when/if ever you adopt ES6.
it makes diffing harder, eg from

var i, j;

to

var i = 0;
var j;

it only saves 63 bytes when minified, or 0.04% of Chart.min.js, which I don't think outweighs the above.

simonbrunel · 2017-02-27T14:11:14Z

Thanks @tredston for the changes,

one-var and vars-on-top are not enforced because we changed linting rules after years of development and it would have required to much change (that's why it's not applied everywhere).
that's a point, we still use var not const and IMO it's safer to have var declared at the top of the scope it's introduced.
not sure I follow that point.
and from a global perspective? I agree that there is room for refactor all around the code, so it may sound useless but actually it makes a small appreciable difference when minified.
kind related: IIRC in QML JavaScript engine, declaring a var inside a loop is (was?) a perf killer, so still better to have it declared outside.

Anyway, that's minor, we can still rework that later, too bad that you removed your last commit implementing these changes ;)

hmcfletch · 2017-02-27T16:58:50Z

I pulled this branch to use on my own site because I was having the overlapping tick issue with some of my time scales. It seems that instead of picking better start and end values to allow for evenly placed ticks, it simply drops what would have been the second to last tick. The makes for a much larger gap between the last two ticks on the axis when displayed.

It is definitely better than the current situation, but there is still an underlying issue that needs to be resolved.

etimberg · 2017-02-28T02:07:31Z

@hmcfletch is it possible to drop a test case here that uses this branch?

hmcfletch · 2017-02-28T16:32:03Z

Working on a deploy today, but i'll see what i can do.

etimberg · 2017-03-08T02:41:30Z

@hmcfletch were you able to make any progress on a test case for the issue you were seeing?

hmcfletch · 2017-03-08T16:48:42Z

Here is the data I am working with:

Screenshot of Chart
https://www.evernote.com/shard/s63/sh/f8cf9342-d934-43e8-820d-6bc4d49cf862/9edfdb6a5428fbcb

Chart Data

{
   "labels":[
      "2017-02-03T00:00:00.000Z",
      "2017-02-04T00:00:00.000Z",
      "2017-02-06T00:00:00.000Z",
      "2017-02-07T00:00:00.000Z",
      "2017-02-08T00:00:00.000Z",
      "2017-02-09T00:00:00.000Z",
      "2017-02-10T00:00:00.000Z",
      "2017-02-11T00:00:00.000Z",
      "2017-02-13T00:00:00.000Z",
      "2017-02-14T00:00:00.000Z",
      "2017-02-15T00:00:00.000Z",
      "2017-02-16T00:00:00.000Z",
      "2017-02-17T00:00:00.000Z",
      "2017-02-19T00:00:00.000Z",
      "2017-02-20T00:00:00.000Z",
      "2017-02-21T00:00:00.000Z",
      "2017-02-22T00:00:00.000Z",
      "2017-02-23T00:00:00.000Z",
      "2017-02-24T00:00:00.000Z",
      "2017-02-26T00:00:00.000Z",
      "2017-02-27T00:00:00.000Z",
      "2017-02-28T00:00:00.000Z",
      "2017-03-01T00:00:00.000Z"
   ],
   "datasets":[
      {
         "fill":false,
         "borderWidth":2,
         "borderColor":"rgba(145, 226, 254, 1)",
         "pointBackgroundColor":"rgba(37, 198, 254, 1)",
         "pointHoverBackgroundColor":"rgba(0, 237, 164, 1)",
         "data":[
            {
               "x":"2017-02-02",
               "y":168244
            },
            {
               "x":"2017-02-03",
               "y":168306
            },
            {
               "x":"2017-02-05",
               "y":168353
            },
            {
               "x":"2017-02-06",
               "y":168432
            },
            {
               "x":"2017-02-07",
               "y":168476
            },
            {
               "x":"2017-02-08",
               "y":168518
            },
            {
               "x":"2017-02-09",
               "y":168596
            },
            {
               "x":"2017-02-10",
               "y":168650
            },
            {
               "x":"2017-02-12",
               "y":168727
            },
            {
               "x":"2017-02-13",
               "y":168780
            },
            {
               "x":"2017-02-14",
               "y":168820
            },
            {
               "x":"2017-02-15",
               "y":168860
            },
            {
               "x":"2017-02-16",
               "y":168926
            },
            {
               "x":"2017-02-18",
               "y":168951
            },
            {
               "x":"2017-02-19",
               "y":169032
            },
            {
               "x":"2017-02-20",
               "y":169078
            },
            {
               "x":"2017-02-21",
               "y":169125
            },
            {
               "x":"2017-02-22",
               "y":169155
            },
            {
               "x":"2017-02-23",
               "y":169193
            },
            {
               "x":"2017-02-25",
               "y":169277
            },
            {
               "x":"2017-02-26",
               "y":169300
            },
            {
               "x":"2017-02-27",
               "y":169330
            },
            {
               "x":"2017-02-28",
               "y":169344
            }
         ]
      }
   ]
}

Chart Options

{
  "responsive": true,
  "legend": {
    "display": false
  },
  "tooltips": {
    "callbacks": {},
    "displayColors": false
  },
  "scales": {
    "yAxes": [
      {
        "type": "linear",
        "ticks": {
          "beginAtZero": false,
          "fontColor": "#7b8994"
        }
      }
    ],
    "xAxes": [
      {
        "ticks": {
          "minRotation": 45,
          "fontColor": "#7b8994"
        },
        "type": "time",
        "time": {
          "format": "YYYY-MM-DD",
          "tooltipFormat": "ll HH:mm"
        }
      }
    ]
  }
}

etimberg · 2017-03-14T22:45:41Z

@tredston @hmcfletch the behaviour you're seeing is happening due to https://github.com/tredston/Chart.js/blob/time-scale-improvements/src/scales/scale.time.js#L185-L189

I can't recall what that was originally designed to achieve. I think it was something to always ensure the last tick is present if the size of the axis is not a multiple of the step size.

Options I see:

leave it as is (regression from current behaviour)
Always push realMax if it is not the last value in the array (don't want any duplicate tick marks).

I personally prefer option 2. @simonbrunel thoughts? @tredston could you make those changes?

etimberg · 2017-03-14T22:47:09Z

I tried changing that if to:

if (ticks[ticks.length - 1] !== realMax) {
    ticks.push(realMax);
}

and it matched v2.5

etimberg · 2017-03-15T21:06:14Z

@tredston one thing to ask as well when you're making that last change (the one in my previous comment). If you can rebase on master to fix the test file conflict this will be good to merge 😄

Months, quarters and years have non-constant numbers of seconds. A scale that's linear WRT milliseconds produces incorrect tick labels due to the label formatting losing precision (eg year labels lose month and day so a label of 2016-12-32 displays as 2016 instead of 2017).

As in v2.5

tredston · 2017-04-01T17:42:11Z

That code was meant to avoid adding the penultimate tick if it was in danger of overlapping with the last tick. Seemingly label rotation was not accounted for.

Requested change applied and rebased onto master.

etimberg · 2017-04-01T21:12:39Z

Thanks @tredston

@simonbrunel shall we merge this?

simonbrunel · 2017-04-02T08:30:43Z

Sure!

tredston force-pushed the time-scale-improvements branch from 0c82865 to ccbdaf7 Compare February 15, 2017 18:39

tredston changed the title ~~Time scale improvements~~ [WIP] Time scale improvements Feb 15, 2017

tredston force-pushed the time-scale-improvements branch from ccbdaf7 to dfefaa3 Compare February 15, 2017 18:50

tredston changed the title ~~[WIP] Time scale improvements~~ Time scale improvements Feb 15, 2017

etimberg requested review from simonbrunel, tannerlinsley, etimberg and panzarino February 15, 2017 22:37

etimberg requested changes Feb 15, 2017

View reviewed changes

tredston force-pushed the time-scale-improvements branch 2 times, most recently from bf77bb6 to 75feb75 Compare February 16, 2017 00:37

tredston force-pushed the time-scale-improvements branch 2 times, most recently from 7190f04 to 8a55cce Compare February 17, 2017 00:18

etimberg approved these changes Feb 17, 2017

View reviewed changes

etimberg added this to the Version 2.6 milestone Feb 17, 2017

panzarino approved these changes Feb 17, 2017

View reviewed changes

simonbrunel requested changes Feb 21, 2017

View reviewed changes

tredston force-pushed the time-scale-improvements branch 5 times, most recently from b10b545 to 2f0e1cb Compare February 27, 2017 13:33

simonbrunel approved these changes Feb 27, 2017

View reviewed changes

etimberg and others added 5 commits April 1, 2017 18:04

Make parseTime private

01127a6

start on fixing time scale

8a2f453

Reimplement existing functionality

f0b6a9c

Tidy tests

191fbf0

tredston force-pushed the time-scale-improvements branch 2 times, most recently from 531ecf3 to 7271e16 Compare April 1, 2017 17:14

Re-implement tick generation

9e65c41

As in v2.5

tredston force-pushed the time-scale-improvements branch from 7271e16 to 9e65c41 Compare April 1, 2017 17:25

simonbrunel mentioned this pull request Apr 2, 2017

v2.5.0 performance issue? #3921

Closed

etimberg merged commit 35dcfe0 into chartjs:master Apr 2, 2017

tredston deleted the time-scale-improvements branch April 2, 2017 18:01

simonbrunel added the type: enhancement label May 13, 2017

etimberg mentioned this pull request May 21, 2017

Time Scale not calculating diffs correctly #3673

Closed

lenyy mentioned this pull request Sep 14, 2017

Timeline Chart type #3574

Closed

benmccann mentioned this pull request Dec 28, 2017

Fix for zoom and pan on time axis chartjs/chartjs-plugin-zoom#105

Merged

		@@ -8,34 +8,42 @@ module.exports = function(Chart) {

		var helpers = Chart.helpers;
		var time = {

Time scale improvements #3914

Time scale improvements #3914

Conversation

tredston commented Feb 15, 2017 • edited Loading

etimberg commented Feb 15, 2017

etimberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tredston Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Feb 16, 2017

tredston commented Feb 17, 2017

etimberg left a comment

Choose a reason for hiding this comment

simonbrunel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tredston commented Feb 27, 2017 • edited Loading

simonbrunel commented Feb 27, 2017

hmcfletch commented Feb 27, 2017

etimberg commented Feb 28, 2017

hmcfletch commented Feb 28, 2017

etimberg commented Mar 8, 2017

hmcfletch commented Mar 8, 2017

etimberg commented Mar 14, 2017

etimberg commented Mar 14, 2017 • edited Loading

etimberg commented Mar 15, 2017

tredston commented Apr 1, 2017

etimberg commented Apr 1, 2017

simonbrunel commented Apr 2, 2017

tredston commented Feb 15, 2017 •

edited

Loading

tredston Feb 16, 2017 •

edited

Loading

tredston commented Feb 27, 2017 •

edited

Loading

etimberg commented Mar 14, 2017 •

edited

Loading