World calendars #1220

alexcjohnson · 2016-12-04T05:18:12Z

@etpinard @n-riesco plots all around the world!

A few TODOs left:

test against gl2d and gl3d plots
test against candlestick and OHLC
get Chinese into world-calendars ( @n-riesco is working on this)
test global layout.calendar

The bulk of this PR is in the two "add world calendar support" commits, but it doesn't make sense to look at the two of them separately, the first is incomplete and did some things wrong that the second completed and fixed.

and hopefully faster than built-in .map

I still can't tell why this is passing as a test when it fails for me in the dashboard (because of timezone conversion)

alexcjohnson · 2016-12-04T06:13:08Z

src/plots/cartesian/set_convert.js

+    /*
+     * wrapped dateTime2ms that:
+     * - accepts ms numbers for backward compatibility
+     * - inserts a dummy arg so calendar is the 3rd arg (see notes below).


There's a lot of rearrangement in this commit, but there are two key points. This is one: calendar is always the 3rd argument to these functions - an extremely unfortunate necessity, since we need to allow different calendar systems on the same axis.

~~Why can't calendar be set in the closure?~~

Answer in #1220 (comment)

you mean the setConvert closure? Because each trace can apply its own calendar to this axis and setConvert only happens per axis. But at least the axis internals only need to pass the first arg because it defaults to ax.calendar.

I knew there was a good reason. Thanks for the info!

I still think we could come up with a better pattern here. Having to remember to pass ax.calendar in the trace module (or transform?) calc steps could potentially produce bugs in future PRs.

Let me test out a few things. Anyhow, I'm ok with the current implementation.

yeah, it's a painful pattern for sure. A lot of trace types never see it though, as it's centralized in ax.makeCalcdata which gets the trace and data attribute as args already so it can look up the calendar. But if anyone can think of a better pattern I would be happy to refactor (after a bit of a break 😅 )

But - and this is perhaps an important point - the way I have it implemented traces do NOT inherit the axis calendar, so actually you never need to pay attention to ax.calendar, just the calendar that's specified in the trace itself. The only calendar inheritance is layout.calendar which sets the default for everything else. That's intended for what I expect will really be the most common use case of this, people really living by one of these calendars who want all their date data and all their date axes to use that calendar.

alexcjohnson · 2016-12-04T06:15:21Z

src/plots/cartesian/set_convert.js

-     * fn to make sure range is a couplet of valid & distinct values
+     * now type-specific conversions for **ALL** other combinations
+     * they're all written out, instead of being combinations of each other, for
+     * both clarity and speed.


... and this is the other key change here: now all possible conversions between d, r, c, l, and p are generated (so you don't need to worry about whether the one you want exists), and they're simplified as much as possible, separately for each axis type.

That's nice.

I've seen some users use the ax.?2? methods in their projects (unfortunately 😏 ), not having to worry about which method is defined should make their lives easier.

alexcjohnson · 2016-12-04T06:21:55Z

src/lib/index.js

+        out = new Array(len);
+    for(var i = 0; i < len; i++) out[i] = func(array[i], x1, x2);
+    return out;
+};


because of the extra args to the axis conversion functions, statements like ax.range.map(ax.r2l) would behave... strangely to say the least. This was actually already true for log axes, but it didn't seem to have bitten us yet. This helper fixes that and lets you pass the extra (constant) args straight through. Not super excited about its name, but I wanted to make sure people didn't use it in place of built-in map without understanding it first, that's why I didn't just call it lib.map.

alexcjohnson · 2016-12-04T06:26:19Z

test/image/mocks/world-cals.json

+            "xaxis": "x2",
+            "yaxis": "y2",
+            "ycalendar": "hebrew",
+            "name": "hebrew histogram<br>jalali filtered",


and displayed against nepali dates - 3 calendars applied to one array!

Nice. We should test the Chinese calendar in this mock too.

cc @n-riesco

etpinard

Oof what an effort 💪 Way to bulldoze through all the traces types 🚚

The new attributes look good. That said, I'm not particular happy with how intrusive world calendars are.

At the moment, world calendars are included in the Lib module meaning that all plotly.js bundles (even custom-made bundles that don't require trace types with support with date axes e.g. geo, ternary traces). That means a lot of extra bytes for everyone. Running npm run build on this branch shows that all our minified bundles will a little under 200K bigger . That a lot of bytes for arguably a niche feature 🍔

That said, as we are late in releasing v1.21.0, I'm ok with this iteration.

Down the road (but hopefully soon), we should require world-calendars in a component module. In this component module, all the calendar attributes would be declared along with the hard-coded conversion table and the special formatting logic. Then, lib/dates.js would call Registry.getComponentMethod to use world calendar component only if it is registered. Finally, we register the world calendar component only when trace module that support dates are themselves registered.

etpinard · 2016-12-05T15:30:26Z

src/lib/coerce.js

+        requiredOpts: [],
+        otherOpts: ['dflt'],
+        coerceFunction: function(v, propOut, dflt) {
+            if(v && calendarList.indexOf(v) !== -1) propOut.set(v);


What's your reasoning for adding a valType instead of reusing enumerated?

just so I didn't have to reference the calendar list all the time. (Oh right, if we componentize, that's a second reference to world-calendars)

I'll see what I can do here.

etpinard · 2016-12-05T15:32:46Z

src/lib/dates.js


 // for 2-digit years, the first year we map them onto
 var YFIRST = new Date().getFullYear() - 70;

+// cache world calendars, so we don't have to reinstantiate
+// during each date-time conversion
+var allCals = {};


nice touch 👌

etpinard · 2016-12-05T15:33:49Z

src/lib/dates.js

    if(y.length === 2) {
        y = (Number(y) + 2000 - YFIRST) % 100 + YFIRST;
    }
    else y = Number(y);

+    // new Date uses months from 0; subtract 1 here just so we
+    // don't have to do it again during the validity test below
+    m -= 1;


excellent comment 👍

etpinard · 2016-12-05T15:56:29Z

src/lib/dates.js

+        // do not allow milliseconds (old) or jsdate objects (inherently
+        // described as gregorian dates) with world calendars
+        if(isWorldCalendar(calendar)) {
+            logError('JS Dates and milliseconds are incompatible with world calendars', v);


etpinard · 2016-12-05T16:01:22Z

src/lib/dates.js

+ * to an ugly placeholder
+ */
+var UNKNOWN = '##';
+var d3ToWorldCalendars = {


Where did you grab the list of d3 templates? I'm (a little bit) concerned that d3 might be adding more templates in the future.

https://github.com/d3/d3-3.x-api-reference/blob/master/Time-Formatting.md

Good call - we'll have to check that when we upgrade d3.

etpinard · 2016-12-05T16:51:06Z

package.json

@@ -89,7 +89,8 @@
    "superscript-text": "^1.0.0",
    "tinycolor2": "^1.3.0",
    "topojson-client": "^2.1.0",
-    "webgl-context": "^2.2.0"
+    "webgl-context": "^2.2.0",
+    "world-calendars": "0.1.0"


Arguably wrong (depending on how much you ❤️ npm), but dependencies in libraries are commonly listed with leading ^ .

👍 when we get @n-riesco 's Chinese in world-calendars I'll change the pattern here - and probably bump it to 1.0.0 at that point since it's doing its job.

etpinard · 2016-12-05T16:51:57Z

src/traces/mesh3d/attributes.js

@@ -37,6 +37,9 @@ module.exports = {
            'jointly represent the X, Y and Z coordinates of the nth vertex.'
        ].join(' ')
    },
+    xcalendar: surfaceAtts.xcalendar,
+    ycalendar: surfaceAtts.ycalendar,
+    zcalendar: surfaceAtts.zcalendar,


Hmm. Do dates actually work in mesh3d traces?

Hmm. Do dates actually work in mesh3d traces?

yes

actually it's surface that has a problem with dates, because colorscale and colorbar don't do dates... you'll notice it's there but I had to explicitly include surfacecolor. We could support this too, but another time.

Great. Thanks for checking 👍

etpinard · 2016-12-05T16:53:10Z

test/image/mocks/world-cals.json

+            "xaxis": "x2",
+            "yaxis": "y2",
+            "ycalendar": "hebrew",
+            "name": "hebrew histogram<br>jalali filtered",


Nice. We should test the Chinese calendar in this mock too.

cc @n-riesco

etpinard · 2016-12-05T16:56:15Z

src/plots/cartesian/graph_interact.js

+        // TODO: should we do something special if the axis calendar and
+        // the data calendar are different? Somehow display both dates with
+        // their system names? Right now it will just display in the axis calendar
+        // but users could add the other one as text.
        else d.xLabel = xLabelObj.text;


Right now it will just display in the axis calendar

Sounds to me like the correct solution.

etpinard · 2016-12-05T16:59:15Z

src/traces/bar/calc.js

+        // ideally if we want to make gantt charts or something we'd treat
+        // the actual size (trace.x or y) as time delta but base as absolute
+        // time. But included here for completeness.
+        scalendar = trace.xcalendar;


Setting the size data calendar won't happen by accident. 👍 for honoring it

alexcjohnson · 2016-12-05T18:41:28Z

Down the road (but hopefully soon), we should require world-calendars in a component module.

I think this would be easy, actually... here is the only place we directly call out to world-calendars (though we probably need another in isWorldCalendar) I just don't know the right pattern - @etpinard if you've got a few free cycles would you want to turn this into a pluggable component?

etpinard · 2016-12-05T19:03:46Z

I just don't know the right pattern - @etpinard if you've got a few free cycles would you want to turn this into a pluggable component?

Sure. I'll give this a go this afternoon.

etpinard · 2016-12-05T22:22:15Z

src/traces/scatter/attributes.js

    y: {
        valType: 'data_array',
+        role: 'info',


Nop. data_array attributes get role: 'data' during PlotSchema.get().

oh good catch - mispaste...

alexcjohnson · 2016-12-06T04:16:24Z

src/components/rangeslider/draw.js

@@ -352,7 +353,8 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) {

        mockFigure.layout[oppAxisName] = {
            domain: [0, 1],
-            range: oppAxisOpts.range.slice()
+            range: oppAxisOpts.range.slice(),
+            calendar: oppAxisOpts.calendar


@etpinard apologies for adding a nearly duplicate mock to test this... but since the fix is in draw I didn't really see another way. I tried to at least put both rangesliders together on one plot but it didn't seem to like that at all.

That's fine 👍

By the way, I've been wanting to add support for multiple range sliders per plot for a while now. Would be a nice feature.

alexcjohnson · 2016-12-06T04:29:09Z

src/components/rangeselector/get_update_object.js

            break;

        case 'todate':
            var base2 = d3.time[step].utc.offset(base, -count);

-            range0 = Lib.ms2DateTime(+d3.time[step].utc.ceil(base2));
+            range0 = axisLayout.l2r(+d3.time[step].utc.ceil(base2));


This is a weird one. At first when I played with range selectors it seemed like they just worked ™️ - if only we were that lucky. They worked (to within errors I would not have been able to notice) as long as the range end is valid in the gregorian calendar and the new start is valid in the axis calendar, but there's no guarantee that that will be true in general. The fix here at least means we properly convert between range values and milliseconds.

What it doesn't get right is stepping backward by some months or years - these steps are going to be somewhat different in each calendar.

What it really doesn't get right is month or year to date. It's going to go to the beginning of the Gregorian month or year, which is generally totally different from in the other calendar. I propose to leave this as an open item for now, because it's going to have all sorts of edge cases to sort out (like in the Hebrew calendar, when the year changes at month 7, not month 1) for a niche-upon-niche feature.

I propose to leave this as an open item for now, because it's going to have all sorts of edge cases to sort out

I agree 👍 . But let's make sure the range selector supplyDefaults doesn't allow axis.calendar !== 'gregorian' with axis.rangeselector.buttons[i].stepmode = 'todate'

no year/month todate (but smaller increments are still allowed) range selectors outside Gregorian calendars: 7bd501f

alexcjohnson · 2016-12-06T04:36:37Z

src/lib/dates.js

+ *
+ * special case for world calendars: multiples of 12 are treated as years,
+ * even for calendar systems that don't have (always or ever) 12 months/year
+ * TODO: perhaps we need a different code for year increments to support this?


@etpinard I meant to flag this one for you - there would be something nice even with regular dates about allowing Y1 and Y5 etc instead of requiring M12 and M60 for 1 and 5 year ticks, even though for Gregorian they're equivalent. And if we allow M12 to mean a year in every calendar now it will be tough to undo later if we decide to add Y1. On the other hand, someone actually wanting 12-month ticks in a calendar that doesn't have 12 months seems ridiculously unlikely.

Ok. Can you open an issue about it after this PR is merged?

etpinard · 2016-12-06T14:43:09Z

test/image/mocks/candlestick_double-y-axis.json

-        1452453861917,
-        1471240462978
+        "2016-01-10 14:24:21.917",
+        "2016-08-15 01:54:22.978"
      ]


Looks like we'll need to update the baseline.

Here's the diff on CI:

https://3140-45646037-gh.circle-artifacts.com/0/home/ubuntu/plotly.js/build/test_images_diff/diff-candlestick_double-y-axis.png

- replace layoutNodes list with _module.schema object which allow multiple distinct attribute (e.g. with different description) for each nodes

- ++ allow trace and transform attributes to be filled by component modules.

"calendars" component

use only the calendars we need from 'world-calendars'

n-riesco · 2016-12-08T21:59:29Z

src/lib/dates.js

+            var calInstance = Registry.getComponentMethod('calendars', 'getCal')(calendar);
+            if(isChinese) {
+                var isIntercalary = m.charAt(m.length - 1) === 'i';
+                m = Number(isIntercalary ? m.substr(0, m.length - 1) : m);


I think parseInt(m) woudl work here

ah good call - I always forget about parseInt.

etpinard · 2016-12-08T22:02:58Z

src/components/calendars/index.js

    coptic: '2000-01-03',
-    discworld: '2000-01-01',
+    discworld: '2000-01-03',


bug fix or typo?

see the comment change above - I decided it would be better to keep our Sundays unless and until we support the native weeks in these calendars.

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

test/jasmine/tests/lib_date_test.js

@@ -284,6 +290,55 @@ describe('dates', function() {
                expect(Lib.dateTime2ms(expected_lastinstant, calendar)).toBe(lastInstant, calendar);
            });
        });
+
+        it('should contain canonical ticks sundays, ranges for all calendars', function() {


very nice test 🍻

n-riesco · 2016-12-08T22:05:52Z

test/jasmine/tests/lib_date_test.js

@@ -341,6 +396,51 @@ describe('dates', function() {
        });
    });

+    describe('incrementMonth', function() {
+        it('should include Chinese intercalary months', function() {


I should add a similar tests to world-calendars

etpinard · 2016-12-08T22:06:13Z

package.json

@@ -90,7 +90,7 @@
    "tinycolor2": "^1.3.0",
    "topojson-client": "^2.1.0",
    "webgl-context": "^2.2.0",
-    "world-calendars": "0.1.0"
+    "world-calendars": "^1.0.0"


etpinard · 2016-12-08T22:08:05Z

Looks good to me 💃 after that parseInt patch.

n-riesco · 2016-12-08T22:08:23Z

test/jasmine/tests/lib_date_test.js

+            ];
+            var tick = Lib.dateTime2ms(start, 'chinese');
+            expected.forEach(function(v) {
+                tick = Lib.incrementMonth(tick, 12, 'chinese');


12? I guess incrementMonth does some magic over leap years

yes exactly: #1220 (comment)

n-riesco · 2016-12-08T22:13:29Z

I had a quick look through the Chinese commits (I didn't go through the functions in Lib). LGTM

alexcjohnson added 7 commits December 2, 2016 19:26

simpleMap utility so we don't pass unexpected args

d2a5e4b

and hopefully faster than built-in .map

change gl2d_date_axes mock to date string axis range

6151575

I still can't tell why this is passing as a test when it fails for me in the dashboard (because of timezone conversion)

add world calendar support part 1

4b9edec

overhaul set_convert so I can tell where calendars go

4d8f79a

add world calendar support part 2

84a51c2

world-cals image mock

13cf6b1

fix 3d calendar attributes

165125b

alexcjohnson commented Dec 4, 2016

View reviewed changes

etpinard added feature something new status: in progress labels Dec 5, 2016

etpinard added this to the v1.21.0 milestone Dec 5, 2016

etpinard reviewed Dec 5, 2016

View reviewed changes

fix gl3d with world calendars

86b31ea

alexcjohnson added 2 commits December 5, 2016 15:45

test world-cals in scattergl & gl2d axes

78b6646

test world calendars with finance charts

1b563f5

etpinard reviewed Dec 5, 2016

View reviewed changes

alexcjohnson added 3 commits December 5, 2016 21:51

remove mistaken paste

4e9a632

support world cals in rangesliders

c1c24e8

partial support for range selectors on world calendars

a435981

alexcjohnson commented Dec 6, 2016

View reviewed changes

etpinard reviewed Dec 6, 2016

View reviewed changes

alexcjohnson added 2 commits December 6, 2016 10:48

get the right date string replacement for candlestick mock

6653da7

bigger tolerance on updatemenus_test width test

8e1747f

etpinard added 2 commits December 6, 2016 16:36

generalise plot schema handling of component attributes

61ecd42

- replace layoutNodes list with _module.schema object which allow multiple distinct attribute (e.g. with different description) for each nodes

add schema attributes in calendars module

68af287

- ++ allow trace and transform attributes to be filled by component modules.

etpinard mentioned this pull request Dec 6, 2016

"calendars" component #1230

Merged

etpinard and others added 8 commits December 7, 2016 11:11

move calendar defaults after early return

cb2c54b

Merge pull request #1230 from plotly/world-cals-component

bc457a9

"calendars" component

use only the calendars we need from 'world-calendars'

1444f55

Merge pull request #1237 from plotly/world-cals-trimmed

08f18ca

use only the calendars we need from 'world-calendars'

support chinese calendar

dcddcee

Merge branch 'master' into world-cals

bbb76a4

fix some tests for chinese, and robustify dateTime2ms

8d8e936

update baseline image with chinese calendar

0e05f95

n-riesco reviewed Dec 8, 2016

View reviewed changes

etpinard reviewed Dec 8, 2016

View reviewed changes

n-riesco reviewed Dec 8, 2016

View reviewed changes

etpinard reviewed Dec 8, 2016

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Dec 8, 2016

n-riesco reviewed Dec 8, 2016

View reviewed changes

alexcjohnson added 2 commits December 8, 2016 19:21

perf: parseInt for string-leading int extraction

509f287

prevent non-gregorian month/year todate range selectors

7bd501f

alexcjohnson merged commit 22c144a into master Dec 9, 2016

alexcjohnson deleted the world-cals branch December 9, 2016 01:52

This was referenced Dec 9, 2016

To Show non-Gregorian Calendars #196

Closed

World calendars open items #1241

Closed

filters use valuecalendar and targetcalendar instead of calendar #1253

Merged

etpinard mentioned this pull request Feb 17, 2017

De-circularize plotly.js require statements #236

Closed

alexcjohnson mentioned this pull request Jul 13, 2017

Per-axis separators attribute #1842

Closed

World calendars #1220

World calendars #1220

Conversation

alexcjohnson commented Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard Dec 5, 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 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

Choose a reason for hiding this comment

alexcjohnson commented Dec 5, 2016

etpinard commented Dec 5, 2016

etpinard Dec 5, 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

etpinard Dec 6, 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

Choose a reason for hiding this comment

etpinard commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Dec 8, 2016

alexcjohnson commented Dec 4, 2016 •

edited

Loading

etpinard Dec 5, 2016 •

edited

Loading

etpinard left a comment •

edited

Loading

etpinard Dec 5, 2016 •

edited

Loading

etpinard Dec 6, 2016 •

edited

Loading