Dates as dates #1078

alexcjohnson · 2016-10-25T04:38:14Z

Stop specifying dates as epoch milliseconds wherever possible. For backward compatibility we will still accept milliseconds where we did previously, but in _fullLayout these are all now turned into date strings:

axis ranges
tick0
annotation positions (and arrow tails if axis referenced)
image positions

Introduces yet another letter to our alphabet soup of data conversions for axes: r for range. The goal is to get r as much as possible to match d, the formats we use for incoming data, but it's not there yet (nor will it ever be):

for linear and now date axes, r===d
for log axes, r===l ie you still need to specify log axes (and the other quantities mentioned above) in linearized terms, ie the log10 of the data. In plotly.js V2.0 we plan to change that to r===d as well but for now there is no clear backward-compatible way to do it.
for category axes, r===c ie the serial numbers of the categories, and this will always be the case because we need a continuous variable.

Note that shapes already behave (and always have) the way we intend for r to behave in V2.0 - that is, all positioning info is in data format except on category axes. So in V2.0 we can clean them up a bit to explicitly use the r machinery. I made some changes in there to prepare for this, so that log looks like the odd-man-out, rather than category.

Also contains some random cleanup to our date systems:

extend support from -9999-01-01 00:00:00.0000 to 9999-12-31 23:59:59.9999 - note that this uses the ISO-8601 convention in which there is a year zero, in contrast to the BC/AD numbering system which has no year zero. Years should always be specified with 4 digits, though we accept 2-digit years with a sliding range now-70 to now+29 years.
we never report a date that's truncated after hours (like 2016-20-24 19) although we will still accept this as data. (fixes Invalid Date format in Hover data when full hour (0 mins) #911 )
tests of all the date conversions!
There seems to have been another bug, manifest in the shapes.png image test which was rendered incorrectly before but correctly now. Since I only noticed this late in the refactor I'm not sure what was causing it...

This refactor is also a prerequisite to using UTC internally (which will fix several other issues in the better dates milestone around time zones and daylight saving time) but note that for backward compatibility, when we do that we will still use local time if users specify dates as milliseconds - which we've always done even though it means these plots are not portable across time zones! But at least after we do this when users do specify dates as strings the plots will be portable. I'm planning to do this next.

@etpinard my apologies, most of this is in one big commit that I couldn't figure out how to break up...

…stants

and introduce new 'r' conversion mode for ranges: can be the same as 'd' (for linear and date), 'l' (for log, for now!) or 'c' (for category, this will have to stay different from 'd' forever so it's continuous)

for axis ranges, annotation & image positions... everything that's an absolute value, not a delta (so dtick is not changed, for example) Apologies in advance to reviewers, I couldn't see a way to break this up.

(for me alexcjohnson anyway)

etpinard · 2016-10-25T21:28:54Z

src/lib/clean_number.js

+ */
+module.exports = function cleanNumber(v) {
+    if(typeof v === 'string') {
+        v = v.replace(FRONTJUNK, '').replace(ENDJUNK, '');


That gets my vote for variable name of the year 🏆

etpinard

Very impressive PRs.

💯 bonus points for cleaning up the gl2d logic.

No 🔴 flags caught my 👀 on the first pass.

But, as the PR impacts a lot of internal and external (hello streambed) plotly components, we should before merging:

npm link this branch in streambed and run the toolpanel and workspace tests there @alexcjohnson I can show how to do this on slack.
make sure animations behave as before @rreusser would you mind fetching this branch and trying out of few mocks?
test drive gl3d / gl2d - I'll take care of that.

Moreover,

we should document what breaking changes to make in v2 in wishlist for potential breaking changes since v1 #420

etpinard · 2016-10-25T21:32:47Z

src/lib/constants.js

+'use strict';
+
+
+module.exports = {


Oh you added a src/lib/constants.js file, there's also a src/constants/ directory in case you haven't noticed.

I'm exactly not sure where to draw the line between the two. Maybe putting this in src/constants/numerical.js would be best. But this is non- ⛔ ; I'll let you decide what's best.

Ah OK - no I hadn't noticed that dir, I'll move it in there. I made the file to get BADNUM out of src/plots/cartesian/constants.js, perhaps eventually this should move there too.

etpinard · 2016-10-25T21:33:46Z

src/lib/dates.js

-    try {
-        if(s.getTime) return +s;
-    }
-    catch(e) {


very nice here. try-catch blocks are sloooooooow.

etpinard · 2016-10-25T21:37:03Z

src/plots/cartesian/axes.js

@@ -528,6 +599,8 @@ axes.autoBin = function(data, ax, nbins, is2d) {
 axes.calcTicks = function calcTicks(ax) {
    if(ax.tickmode === 'array') return arrayTicks(ax);

+    var rng = ax.range.map(ax.r2l);


clever 👍

etpinard · 2016-10-25T21:38:27Z

src/plots/cartesian/constants.js

+    // last resort axis ranges for x, y, and date axes if we have no data
+    DFLTRANGEX: [-1, 6],
+    DFLTRANGEY: [-1, 4],
+    DFLTRANGEDATE: ['2000-01-01', '2001-01-01'],


etpinard · 2016-10-25T21:39:21Z

src/plots/cartesian/set_convert.js

 var axisIds = require('./axis_ids');


 /**
- * Define the conversion functions for an axis data is used in 4 ways:
+ * Define the conversion functions for an axis data is used in 5 ways:


good 👁️

etpinard · 2016-10-25T21:54:45Z

src/lib/index.js

@@ -35,7 +35,6 @@ var datesModule = require('./dates');
 lib.dateTime2ms = datesModule.dateTime2ms;
 lib.isDateTime = datesModule.isDateTime;
 lib.ms2DateTime = datesModule.ms2DateTime;
-lib.parseDate = datesModule.parseDate;


etpinard · 2016-10-25T22:04:08Z

src/components/annotations/annotation_defaults.js

-    var axLetters = ['x', 'y'];
+    var axLetters = ['x', 'y'],
+        arrowPosDflt = [-10, -30],
+        tdMock = {_fullLayout: fullLayout};


Those td scare the 💩 out of me. Would you mind renaming this gdMock ?

Halloween is almost upon us, so maybe I'll scare you even more by swapping td for gd EVERYWHERE that remnant of streambed was lingering. EXORCISE THE DEMON! 5ccd083

etpinard · 2016-10-25T22:05:48Z

test/jasmine/tests/annotations_test.js

-
-            var annotationIn = {
-                annotations: [{ showarrow: true, axref: 'x', ayref: 'y', x: '2008-07-01', ax: '2004-07-01', y: 0, ay: 50}]
+        it('should convert ax/ay date coordinates to date string if tail is in milliseconds and axis is a date', function() {


etpinard · 2016-10-25T22:07:37Z

src/components/shapes/shape_defaults.js

+                in0 = shapeIn[attr0],
+                in1 = shapeIn[attr1];
+            shapeIn[attr0] = pos2r(shapeIn[attr0], true);
+            shapeIn[attr1] = pos2r(shapeIn[attr1], true);


Would you mind making sure this logic is tested in shapes_test.js ?

good call - it was actually broken 🙈
e63ea3b

etpinard · 2016-10-25T22:08:34Z

test/jasmine/tests/mapbox_test.js

@@ -449,7 +449,7 @@ describe('mapbox plots', function() {

            var divStyle = mapInfo.div.style;
            ['left', 'top', 'width', 'height'].forEach(function(p, i) {
-                expect(parseFloat(divStyle[p])).toBeWithin(dims[i], 5);
+                expect(parseFloat(divStyle[p])).toBeWithin(dims[i], 8);


Oh nice. Does this make the full npm run citest-jamine run without errors for you?

npm-run test-jasmine runs without errors after this change (though that skips a bunch of the tests), but citest-jasmine still seems to fail.

other than the occasional

Uncaught TypeError: gd.emit is not a function at /var/folders/z4/src/plots/mapbox/mapbox.js:147:0

that still causes test-jasmine to fail intermittently

etpinard · 2016-10-25T22:42:53Z

@alexcjohnson and I should add: I'm thinking about not including this PR in 1.19.0 set to be released this Thursday.

I would prefer making this PR a stepping-stone for non-western set for 1.20.0 - if you don't mind of course.

alexcjohnson · 2016-10-26T00:21:02Z

I would prefer making this PR a stepping-stone for non-western set for 1.20.0 - if you don't mind of course.

Fine by me. 20 can be the big date overhaul.

except for image server compatibility, we leave td in plotTile

alexcjohnson · 2016-10-26T03:40:21Z

@etpinard I think I've addressed your comments. I'll ping you tomorrow about npm link.

alexcjohnson · 2016-11-09T19:29:38Z

(after I fix the jasmine test I broke that is)

rreusser · 2016-11-09T20:04:32Z

FYI: your tests will probably continue to fail due to a backwards-incompatible eslint upgrade that seems to have just taken effect on circle-ci. @etpinard is fixing now. Solution will be an eslint upgrade.

etpinard · 2016-11-09T20:20:07Z

Solution will be an eslint upgrade

merging master should do the trick now.

etpinard · 2016-11-10T14:59:58Z

src/plots/cartesian/layout_attributes.js

+            'gives ticks linearly spaced in value (but not position).',
+            'For example `tick0` = 0.1, `dtick` = *L0.5* will put ticks at 0.1, 0.6, 1.1, 1.6 etc.',
+            'To show powers of 10 plus small digits between, use *D1* (all digits) or *D2* (only 2 and 5).',
+            '`tick0` is ignored for *D1* and *D2*.',


Nice. Is this a new or a current (but hidden) feature?

These have always existed behind the scenes but I think there were some bugs if you tried to use them yourself as opposed to getting them automatically. Now they work manually, and are tested at the level of supplyDefaults (which is where the bugs were before, I think) though at some point it might be worth adding tests of the functionality when used manually.

etpinard · 2016-11-10T15:00:21Z

src/plots/cartesian/tick_value_defaults.js

@@ -22,22 +22,53 @@ module.exports = function handleTickValueDefaults(containerIn, containerOut, coe
    }

    if(Array.isArray(containerIn.tickvals)) tickmodeDefault = 'array';
-    else if(containerIn.dtick && isNumeric(containerIn.dtick)) {


ha. I guess a new feature. 😌

... and probably one of the most useful new features of the passed few months 🏆

etpinard

@alexcjohnson this is looking great. I'm particularly excited about configurable tick0 + dtick for log and date axes.

etpinard · 2016-11-10T15:02:23Z

src/plots/cartesian/tick_value_defaults.js

+        // dtick is usually a positive number, but there are some
+        // special strings available for log or date axes
+        // default is 1 day for dates, otherwise 1
+        var dtickDflt = (axType === 'date') ? 86400000 : 1;


that 1 day in ms 8640000 would fit well in the new src/constants/numerical

blocking change

etpinard · 2016-11-10T15:05:29Z

src/plots/cartesian/tick_value_defaults.js

+                    // "L<f>" gives ticks linearly spaced in data (not in position) every (float) f
+                    (axType === 'log' && prefix === 'L') ||
+                    // "D1" gives powers of 10 with all small digits between, "D2" gives only 2 and 5
+                    (axType === 'log' && prefix === 'D' && (dtickNum === 1 || dtickNum === 2))


Nice. This is going to make a lot of users happy. You wouldn't believe how many people have ask for custom date tick labels. This is way better then tickvals + ticktext!

etpinard · 2016-11-10T15:09:31Z

src/plots/cartesian/tick_value_defaults.js

@@ -22,22 +22,53 @@ module.exports = function handleTickValueDefaults(containerIn, containerOut, coe
    }

    if(Array.isArray(containerIn.tickvals)) tickmodeDefault = 'array';
-    else if(containerIn.dtick && isNumeric(containerIn.dtick)) {


... and probably one of the most useful new features of the passed few months 🏆

etpinard · 2016-11-10T15:12:14Z

src/plots/cartesian/axes.js

+var ONEDAY = 86400000,
+    ONEHOUR = 3600000,
+    ONEMIN = 60000,
+    ONESEC = 1000;


Let's put these src/constants/numerical

blocking change (bis)

etpinard · 2016-11-10T15:15:14Z

src/plots/cartesian/axes.js

@@ -961,7 +985,7 @@ axes.tickFirst = function(ax) {
 var yearFormat = d3.time.format('%Y'),
    monthFormat = d3.time.format('%b %Y'),
    dayFormat = d3.time.format('%b %-d'),
-    hourFormat = d3.time.format('%b %-d %Hh'),
+    yearMonthDayFormat = d3.time.format('%b %-d, %Y'),


I can tell someone is about to add layout.xaxis.tickdateformat ...

axis.tickformat does this already... but I see the docs do not reflect this. I'll update the description accordingly. We use d3.time.format with an addition for fractional seconds.

etpinard · 2016-11-10T15:17:16Z

test/jasmine/tests/axes_test.js

@@ -1392,4 +1392,88 @@ describe('Test axes', function() {
            expect(ax._max).toEqual([{val: 6, pad: 15}]);
        });
    });
+
+    describe('calcTicks', function() {


Amazing tests. So clear.

etpinard · 2016-11-10T15:20:37Z

test/image/mocks/layout_image.json

@@ -4,17 +4,22 @@
      "x": [1,2,3],
      "y": [1,2,3]
    }, {
-      "x": [1,2,3],
-      "y": [1,2,3],
+      "x": ["2001-01-01","2002-01-01","2003-01-01"],


I see you're catching on to the reusing existing mocks philosophy. Thanks! Our CI test time will thank you too

etpinard · 2016-11-10T15:27:30Z

src/components/annotations/draw.js

@@ -579,21 +579,21 @@ function drawOne(gd, index, opt, value) {
                        ann.call(Lib.setTranslate, xcenter, ycenter);

                        update[annbase + '.x'] = xa ?
-                            (options.x + dx / xa._m) :
+                            xa.l2r(xa.p2l(xa.l2p(xa.r2l(options.x)) + dx)) :


can you factor that out into something like:

function addPxDistToRange(ax, v, dv) { return ax.l2r(ax.p2l(ax.l2p(ax.r2l(v)) + dv)) }

or a better name.

blocking change

maybe I'll just make ax.r2p and ax.p2r, which I just omitted because I didn't have a use case for it for a while, but now there are a couple of places that use it.

maybe I'll just make ax.r2p and ax.p2r

Sounds good!

etpinard · 2016-11-10T15:33:08Z

src/components/annotations/draw.js

                            ((arrowX + dx - gs.l) / gs.w);
                        update[annbase + '.y'] = ya ?
-                            (options.y + dy / ya._m) :
+                            ya.l2r(ya.p2l(ya.l2p(ya.r2l(options.y)) + dy)) :


Have you tested shapes under { editable: true }? @n-riesco did add some jasmine test cases for them, but you can never be too sure.

Tested. Editable shapes on date axes work just fine ✅

Oh interesting... didn't even know they were editable but yes, seems to work 👍

etpinard · 2016-11-10T15:33:55Z

test/jasmine/tests/annotations_test.js


            expect(fullLayout.xaxis.range).toBeCloseToArray(x, PREC, '- xaxis');
            expect(fullLayout.yaxis.range).toBeCloseToArray(y, PREC, '- yaxis');
-            expect(fullLayout.xaxis2.range).toBeCloseToArray(x2, PREC2, 'xaxis2');
-            expect(fullLayout.yaxis2.range).toBeCloseToArray(y2, PREC, 'yaxis2');
+            expect(dateAx.range.map(dateAx.r2l)).toBeCloseToArray(x2.map(dateAx.r2l), PRECX2, 'xaxis2 ' + dateAx.range);


thanks 🎉

etpinard · 2016-11-10T15:38:01Z

@alexcjohnson I just submitted my review. I'll spend the next 30 minutes test 🚗 this PR.

etpinard · 2016-11-10T18:13:43Z

src/plots/cartesian/axes.js

-    return Lib.coerce(containerIn, containerOut, attrDef, refAttr);
+        if(ax.type === 'category') {
+            // if position is given as a category name, convert it to a number
+            if(typeof pos === 'string' && (ax._categories || []).length) {


maybe add a note that ax._categories is filled during the calc step only (by ax.makeCaldata to be more precise), so ax_categories is in fact [] during Plotly.supplyDefaults

So, the block below only makes sense for Axes.coercePosition calls in the annotations and shapes draw methods.

etpinard · 2016-11-10T20:55:49Z

💃

sglyon · 2016-11-11T17:43:06Z

Does this PR by chance recognize "2013-10-04T22:23:00" as date time?

I know that as of v1.19.0 it plotly.js recognizes "2013-10-04 22:23:00" as a date time, but not the version with the T in the middle. this is a bit unfortunate because the version with the T adheres to the examples for ISO-8601.

alexcjohnson · 2016-11-14T16:49:28Z

@spencerlyon2

Does this PR by chance recognize "2013-10-04T22:23:00" as date time?

No, that's not in this PR, but it's on my list to add soon #1003

chriddyp · 2016-11-14T23:49:27Z

Heads up @cldougl ! When this merges we'll be able to remove all milliseconds from our documentation examples 🎉

alexcjohnson added 8 commits October 14, 2016 16:51

improve dateTime2ms edge cases and standardize BADNUM and FP_SAFE con…

d4bb89d

…stants

switch date axes to use date strings for range and tick0

9e27955

and introduce new 'r' conversion mode for ranges: can be the same as 'd' (for linear and date), 'l' (for log, for now!) or 'c' (for category, this will have to stay different from 'd' forever so it's continuous)

Merge branch 'master' into dates-as-dates

00138aa

axe parseDate - moved to streambed

9784b3b

refactor dates handling to use date strings as much as possible

1db4448

for axis ranges, annotation & image positions... everything that's an absolute value, not a delta (so dtick is not changed, for example) Apologies in advance to reviewers, I couldn't see a way to break this up.

Merge branch 'master' into dates-as-dates

5ff9722

wider acceptance range for mapbox plot size test so it works locally

c045bc5

(for me alexcjohnson anyway)

fix for range slider tests with new axis range machinery

3dd294f

etpinard added bug something broken feature something new status: in progress labels Oct 25, 2016

fix date axis ranges and date interactions in gl2d

451ee24

etpinard reviewed Oct 25, 2016

View reviewed changes

etpinard suggested changes Oct 25, 2016

View reviewed changes

alexcjohnson added 4 commits October 25, 2016 20:47

Merge branch 'master' into dates-as-dates

9ccde7e

lib/constants -> constants/numerical

51a0563

td -> gd once and for all

5ccd083

except for image server compatibility, we leave td in plotTile

test and fix shape default positioning

e63ea3b

alexcjohnson mentioned this pull request Oct 26, 2016

wishlist for potential breaking changes since v1 #420

Closed

15 tasks

fix date and log axis manual tick0/dtick handling

2e9dbad

etpinard added this to the v1.20.0 milestone Oct 28, 2016

alexcjohnson added 6 commits October 28, 2016 15:14

fix handling of default tick0 with dtick for date axes

37d52fe

get rid of '2012-01-22 12h' tick format and smarter date suffix handling

55b313c

fix #1058 - make sure no date ever returns sub-100microsec precision

b7121c3

Merge branch 'master' into dates-as-dates

03b4a0c

fix _forceTick0 error

dd0940a

update baseline with new date tick format

f629f71

update jasmine test of annotation visibility & autorange

7679483

alexcjohnson added 2 commits November 9, 2016 15:27

Merge branch 'master' into dates-as-dates

0d61fcc

merge lint

5e11571

etpinard reviewed Nov 10, 2016

View reviewed changes

etpinard suggested changes Nov 10, 2016

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Nov 10, 2016

etpinard reviewed Nov 10, 2016

View reviewed changes

alexcjohnson added 3 commits November 10, 2016 15:28

add ax.r2p and ax.p2r

d807259

date interval milliseconds constants

05c762d

documentation updates

1b0e133

etpinard approved these changes Nov 10, 2016

View reviewed changes

alexcjohnson merged commit 89c68e2 into master Nov 10, 2016

alexcjohnson deleted the dates-as-dates branch November 10, 2016 21:02

This was referenced Nov 15, 2016

make histogram bins work with date strings #1150

Merged

annotation placement for ordinal axis bugs with version 1.20 #1154

Closed

cldougl mentioned this pull request Nov 16, 2016

remove milliseconds from doc examples plotly/documentation#597

Closed

etpinard mentioned this pull request Nov 17, 2016

issue #1160 fix #1161

Closed

alexcjohnson mentioned this pull request Nov 19, 2016

Date auto-formatting on xaxis #99

Closed

etpinard mentioned this pull request Nov 21, 2016

Fix regression in lib/clean_number #1184

Closed

etpinard mentioned this pull request Feb 8, 2017

Single-datum bar trace with specified time range not showing #1346

Closed

Dates as dates #1078

Dates as dates #1078

Conversation

alexcjohnson commented Oct 25, 2016

Choose a reason for hiding this comment

etpinard left a comment • 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

etpinard commented Oct 25, 2016

alexcjohnson commented Oct 26, 2016

alexcjohnson commented Oct 26, 2016

alexcjohnson commented Nov 9, 2016

rreusser commented Nov 9, 2016

etpinard commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Nov 10, 2016 • 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

etpinard Nov 10, 2016 • 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

etpinard commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 10, 2016

sglyon commented Nov 11, 2016 • edited Loading

alexcjohnson commented Nov 14, 2016

chriddyp commented Nov 14, 2016

etpinard left a comment •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading

etpinard Nov 10, 2016 •

edited

Loading

sglyon commented Nov 11, 2016 •

edited

Loading