Timeseries scale #4364

IlyaBeliaev · 2017-06-12T10:48:37Z

Timeseries scale added, see this issue for more info: #4189

small example here: https://codepen.io/ilyabelyaev/pen/BRvKPw

etimberg · 2017-06-15T11:22:53Z

@IlyaBeliaev I haven't had a chance to look at this in detail yet, but could you rebase this against master?

benmccann · 2017-06-16T17:22:10Z

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

+	<script src="../../../dist/Chart.js"></script>
+	<script src="../../utils.js"></script>
+	<style>
+    canvas {


Looks like there's some indentation here that should be switched from spaces to tabs

benmccann · 2017-06-16T17:22:47Z

samples/scales/timeseries/line.html

+	<script src="../../../dist/Chart.js"></script>
+	<script src="../../utils.js"></script>
+	<style>
+    canvas {


spaces vs tabs

benmccann · 2017-06-16T17:32:23Z

src/scales/scale.timeseries.js

+				allTimestamps = allTimestamps.concat(value);
+			});
+
+			allTimestamps = arrayUnique(allTimestamps).sort(function(a, b) {


I'm not sure we should sort here. I would expect the user to pass the data points in sorted already. If they want to plot in reverse chronological order we should probably let them do that, but sorting would stop that from working

Agreed, we shouldn't sort since.

I guess we should add some config option to allow users to make reverse order. I can't build ticks without sorting here. Because there are a data from all datasets.

Maybe for the time series scale we can just say that scatter is not supported?

Ok, I'm fine saying scatter is not supported

benmccann · 2017-06-16T17:34:50Z

docs/axes/cartesian/timeseries.md

+
+## Date Formats
+
+When providing data for the time scale, Chart.js supports all of the formats that Moment.js accepts. See [Moment.js docs](http://momentjs.com/docs/#/parsing/) for details.


"time scale" -> "timeseries scale"

etimberg · 2017-06-18T20:44:27Z

docs/axes/cartesian/timeseries.md

+    data: data,
+    options: {
+        scales: {
+            xAxes: [{


we should add the type option here to the x axis config

etimberg · 2017-06-18T20:47:20Z

src/scales/scale.timebase.js

+		// Get tooltip label
+		getLabelForIndex: function(index, datasetIndex) {
+			var me = this;
+			var label = me.chart.data.labels && index < me.chart.data.labels.length ? me.chart.data.labels[index] : '';


[nit] me.chart.data is used a couple of times here. I think we should capture it in a local variable for improved minification

etimberg · 2017-06-18T20:47:46Z

src/scales/scale.timeseries.js

+
+		time: {
+			parser: false, // false == a pattern string from http://momentjs.com/docs/#/parsing/string-format/ or a custom callback that converts its argument to a moment
+			format: false, // DEPRECATED false == date objects, moment object, callback or a pattern string from http://momentjs.com/docs/#/parsing/string-format/


do we need to provide this option since it's deprecated?

Hm, It looks like both options have no effect on time series. So I can remove them.

Sounds good

Format is used in helpers.time and it should be removed from both scales at one time.

etimberg · 2017-06-18T20:47:53Z

src/scales/scale.timeseries.js

+			format: false, // DEPRECATED false == date objects, moment object, callback or a pattern string from http://momentjs.com/docs/#/parsing/string-format/
+			unit: false, // false == automatic or override with week, month, year, etc.
+			round: false, // none, or override with week, month, year, etc.
+			displayFormat: false, // DEPRECATED


same with this option?

etimberg · 2017-06-18T20:50:46Z

src/scales/scale.timeseries.js

+			return value;
+		},
+		getBasePixel: function() {
+			return this.bottom;


what will this do for horizontal bar charts using the timeseries axis as the y axis?

It works only on x axis same as time scale.

etimberg · 2017-07-03T11:24:29Z

I think this is ready to be merged (other than the conflict in src/chart.js)

IlyaBeliaev · 2017-07-03T12:12:18Z

it's good! made rebase

simonbrunel

A few feedback.

Wondering if since timeseries is closer to the category logic, why not inheriting category instead of timebase, and have time manipulation method as helpers?

simonbrunel · 2017-07-03T12:27:08Z

docs/axes/cartesian/timeseries.md

@@ -0,0 +1,94 @@
+# TimeSeries Cartesian Axis


Could we find a way to avoid this doc duplication: maybe merge both in the same page (docs/axes/cartesian/time.md) with a special section about the TimeSeries listing differences? or a common page for time base scales?

added timeseries description into time.md

simonbrunel · 2017-07-03T12:27:30Z

samples/samples.js

@@ -117,6 +117,18 @@
 			path: 'scales/time/combo.html'
 		}]
 	}, {
+		title: 'TimeSeries scale',


I'm not sure we need to duplicate the 3 examples, maybe 1 is enough and could be added to the Time Scale group at the end (title: Series (TimeSeries Scale)).

A single example would be fine with me. I would just suggest that we not use the combo chart as the example in that case. We were also talking about following up afterwards in a separate PR with some improvements to the time samples. E.g. make the samples show more points by default and add a dropdown which lets you see samples with minutes, hours, weeks, months, etc. instead of just days

simonbrunel · 2017-07-03T12:30:02Z

src/scales/scale.timebase.js

+
+			return label;
+		},
+    // Function to format an individual tick mark


Indentation++

simonbrunel · 2017-07-03T12:30:25Z

src/scales/scale.timebase.js

+			var label = chartData.labels && index < chartData.labels.length ? chartData.labels[index] : '';
+			var value = chartData.datasets[datasetIndex].data[index];
+
+			if (value !== null && typeof value === 'object') {


if(helpers.isObject(value))

simonbrunel · 2017-07-03T12:31:31Z

src/scales/scale.timebase.js

+
+			var callback = helpers.valueOrDefault(tickOpts.callback, tickOpts.userCallback);
+
+			if (callback) {


return { value: callback? callback(formattedTick, index, ticks) : formattedTick, major: major };

simonbrunel · 2017-07-03T12:31:51Z

src/scales/scale.timebase.js

+		convertTicksToLabels: function() {
+			var me = this;
+			me.ticksAsTimestamps = me.ticks;
+			me.ticks = me.ticks.map(function(tick) {


me.ticks = me.ticks.map(moment).map(me.tickFormatFunction, me);

it's not possible here because moment has constructor with multiple params. See https://momentjs.com/docs/#/parsing/string-formats/

Might be good to add a comment or even better, change it for a vanilla for loop and avoid doubling function calls?

simonbrunel · 2017-07-03T12:32:04Z

src/scales/scale.timebase.js

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


Move that line after all var to help minification

me.displayFormat used inside me.tickFormatFunction which is called on a next line

Not obvious :\

simonbrunel · 2017-07-03T12:33:25Z

src/scales/scale.timeseries.js

+			minUnit: 'millisecond',
+
+			// defaults to unit's corresponding unitFormat below or override using pattern string from http://momentjs.com/docs/#/displaying/format/
+			displayFormats: {


We should really find a way to avoid these options duplication (not sure how though) since both scales inherit the same base time scale, so would make sense to have common options in the base class as well.

Now time and timeseries scale has different displayFormats because timeseries scale does not support tick alignment and major units yet. I'm working on the next PR that adds this functionality to the timeseries scale. There display formats will be same for both scales and then I can move it to helpers.time.

Sounds good!

simonbrunel · 2017-07-03T12:36:32Z

src/scales/scale.timeseries.js

+
+				if (typeof dataset.data[0] === 'object' && dataset.data[0] !== null && me.chart.isDatasetVisible(datasetIndex)) {
+					// We have potential point data, so we need to parse this
+					helpers.each(dataset.data, function(value, dataIndex) {


We should avoid as much as possible to use function based iterations, it's a perf killer especially when iterating lot of data.

replaced with vanilla js loops

simonbrunel · 2017-07-03T12:43:24Z

Also curious how many KB this PR introduces compared to the master Chart.min.js size?

benmccann · 2017-07-03T16:33:13Z

Yeah, timeseries is conceptually part time / part category However, it turns out that it's quite hard to share much code with category. I tried that first I believe, but there were only a few methods that were generally the same and even they had subtle differences that made sharing hard. I think inheriting from time is actually the better way to go as it cuts down more on the amount of code required.

IlyaBeliaev · 2017-07-09T22:40:52Z

Sizes: 155 734 bytes vs 153 051 bytes

IlyaBeliaev · 2017-07-09T22:44:02Z

About a code from category scale. I'm working on the next PR that will add support of tick alignment and major units for timeseries scale. There timeseries scale have an other code to determine tick positions etc. So it was a good decision to inherit it from time scale.

simonbrunel

Thanks @IlyaBeliaev for the updates. Just added a few more feedback that I think should be changed before merging.

There is still some pieces of code I didn't report but would be great to change for performance, minification and readability but I guess I'm too picky and don't want to keep this PR open forever.

Edit: actually, these changes mostly concern the existing code in master you refactored in timebase.

simonbrunel · 2017-07-10T08:29:02Z

samples/samples.js

@@ -105,7 +105,7 @@
 			path: 'scales/logarithmic/scatter.html'
 		}]
 	}, {
-		title: 'Time scale',
+		title: 'Time and TimeSeries scale',


Can we keep title: 'Time scales' and avoid the title on 2 lines?

simonbrunel · 2017-07-10T08:29:08Z

src/scales/scale.time.js

@@ -39,7 +38,7 @@ module.exports = function(Chart) {
 		}
 	};

-	var TimeScale = Chart.Scale.extend({
+	var TimeScale = Chart.TimeScaleBase.extend({
 		initialize: function() {


I think this initialize override needs to be removed (looks exactly the same as the one in TimeScaleBase).

simonbrunel · 2017-07-10T08:29:20Z

src/scales/scale.timeseries.js

+			minUnit: 'millisecond',
+
+			// defaults to unit's corresponding unitFormat below or override using pattern string from http://momentjs.com/docs/#/displaying/format/
+			displayFormats: {


Sounds good!

simonbrunel · 2017-07-10T08:35:12Z

src/scales/scale.timebase.js

+		convertTicksToLabels: function() {
+			var me = this;
+			me.ticksAsTimestamps = me.ticks;
+			me.ticks = me.ticks.map(function(tick) {


Might be good to add a comment or even better, change it for a vanilla for loop and avoid doubling function calls?

simonbrunel · 2017-07-10T08:36:08Z

src/scales/scale.timebase.js

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


Not obvious :\

…rom time.scale;

IlyaBeliaev · 2017-07-11T09:00:22Z

@simonbrunel Thank you for the review. I think I can refactor timebase a bit in a next PR.

benmccann · 2017-07-13T17:20:01Z

src/scales/scale.timeseries.js

+			},
+		},
+		ticks: {
+			autoSkip: true


@IlyaBeliaev comment from @simonbrunel :

I see you turn autoSkip back to true for the timeseries? actually, that's my (hopefully) last issue: since the time scale doesn't support autoSkip, it's false by default, so can't have the same default as your PR (true)

I wasn't sure why you needed to do this as I had it set to false in the original code: https://github.com/chartjs/chartjs-chart-financial/blob/master/src/scale.timeseries.js#L85

Is this something you can change back to false?

See #4507 for details

etimberg mentioned this pull request Jun 14, 2017

Huge gap between last two labels when using maxTicksLimit #4358

Closed

benmccann reviewed Jun 16, 2017

View reviewed changes

etimberg reviewed Jun 18, 2017

View reviewed changes

IlyaBeliaev force-pushed the timeseries-scale branch 2 times, most recently from 23d57fd to 6b9c919 Compare June 28, 2017 11:52

etimberg approved these changes Jul 3, 2017

View reviewed changes

IlyaBeliaev force-pushed the timeseries-scale branch from 7c66743 to 99ee03c Compare July 3, 2017 12:12

simonbrunel requested changes Jul 3, 2017

View reviewed changes

IlyaBeliaev and others added 4 commits July 9, 2017 23:49

TimeSeries scale added; time series refactoring (chartjs#4189)

97cc62b

Minor improvements to samples

bd6b656

minor fixes

f9121c7

docs fix; format removed from config;

de7d098

IlyaBeliaev force-pushed the timeseries-scale branch from 99ee03c to de7d098 Compare July 9, 2017 20:51

IlyaBeliaev added 2 commits July 10, 2017 00:16

docs and samples update

43add34

minor fixes

5b1f038

IlyaBeliaev force-pushed the timeseries-scale branch from 3bfdbc4 to 5b1f038 Compare July 9, 2017 22:36

simonbrunel requested changes Jul 10, 2017

View reviewed changes

tick format loop refactored; samples fix; initialize method removed f…

7de93d6

…rom time.scale;

simonbrunel previously approved these changes Jul 11, 2017

View reviewed changes

benmccann reviewed Jul 13, 2017

View reviewed changes

simonbrunel mentioned this pull request Jul 15, 2017

New time scale ticks.mode and ticks.source options #4507

Merged

IlyaBeliaev closed this Jul 24, 2017


		## Date Formats

		When providing data for the time scale, Chart.js supports all of the formats that Moment.js accepts. See [Moment.js docs](http://momentjs.com/docs/#/parsing/) for details.


		var callback = helpers.valueOrDefault(tickOpts.callback, tickOpts.userCallback);

		if (callback) {

Timeseries scale #4364

Timeseries scale #4364

Conversation

IlyaBeliaev commented Jun 12, 2017

etimberg commented Jun 15, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Jul 3, 2017

IlyaBeliaev commented Jul 3, 2017

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

IlyaBeliaev Jul 9, 2017 • edited

Choose a reason for hiding this comment

simonbrunel Jul 10, 2017 • edited

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

simonbrunel commented Jul 3, 2017

benmccann commented Jul 3, 2017

IlyaBeliaev commented Jul 9, 2017

IlyaBeliaev commented Jul 9, 2017

simonbrunel left a comment • edited

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

simonbrunel Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaBeliaev commented Jul 11, 2017

Choose a reason for hiding this comment

IlyaBeliaev Jul 9, 2017 •

edited

simonbrunel Jul 10, 2017 •

edited

simonbrunel left a comment •

edited

simonbrunel Jul 10, 2017 •

edited