189 ordinal scale domain item ordering #1

monfera · 2016-03-29T15:37:59Z

Initial, minimal changeset for demonstrating the categorymode = 'array' option.

Test:

Plotly.newPlot('embedded-graph', [
            {x: ['a','b',    'd','e','f'    ], y: [100,110,    130,140,150    ]},
            {x: ['a',    'c',    'e','f','g'], y: [101,    121,    141,151,161]}],
            {xaxis: {type: 'category', categorymode: 'array', categorylist: ['a','b','c','d','e','f','g']}}
        );

should output

…adding new usages with (currently) failing test cases

…e of order specification

…c than data coverage

…pletely cover the supplied input data

…g categorymode values

… presumably as part of providing compatibility with an older API version)

etpinard · 2016-03-29T16:43:30Z

src/plots/cartesian/set_convert.js

-            // progressively here as we insert?
+            // it is assumed that this function is being invoked in the
+            // already sorted category order; otherwise there would be
+            // a disconnect between the array and the index returned


@monfera can you elaborate on this?

What else have you tried?

@etpinard I tried things like this:

I've first tried the change inside the suggested place (https://github.com/plotly/plotly.js/blob/bbb8205903cdd34ae019d5cd6a2c197e9a2550e7/src/plots/cartesian/set_convert.js#L179-L194): instead of pushing into the array, I inserted it to ensure orderedness (insertion sort). More accurately, for simplicity, I just sorted upon each insertion to try before doing too much work (paid attention to the fact that [].sort mutates the array).

Then I also quickly tried doing the sort only after the loop that invokes ax.d2c but the issue is, by that point we already have the indices, and a sort renders them obsolete.

Then I spent a bit of time better understanding concepts, the data flow and the various ways it can be invoked (e.g. plotting multiple lines; overplotting onto an already existing plot etc). Learnt that currently, the categorical X axis ordering is totally driven by the order in which the point tuples arrive and made sample plots. Which was the point at which I gave up on this point for modification and went for the lower hanging fruit of array. The other options will, I think, be more intrusive to the data flow.

@etpinard maybe an example is helpful: The first vector has X cat. values ['a', 'c', 'd']. This is already sorted. Then a subsequent series is plotted in the same chart, with values for ['a', 'b', 'c', 'd']. By this point, the function has returned indices such as 0, 1, 2 for ['a', 'c', 'd'], respectively. Then in the next series comes a new point 'b' correctly inserted at index 1 but the previously returned indices will become invalid and the plot will come out nonsensical (more precisely, having checked the plot, the way it came out was consistent with this mechanics).

@etpinard I also considered doing a sorting a lot earlier (more upstream), but it would have issues: 1) the code should possibly reorder only after it's established that it's a categorical axis (it can be user specified or determined heuristically); 2) by that point there are lots of places where _fullData etc. are persisted on objects in the trace order; 3) I believe it would be wrong to change the trace order just for this CR because of the snail trail example and the general possibility that the user does not want to have plotly change the user input order (which is currently implicitly taken as the trace order).

So my current thought is that we need to do the axis tick sorting downstream, nearer the point which is responsible for the d3 data binding order for the axis ticks, and we have to accommodate for the possibility that new lines introduce new points that have to be inserted in lines that have already been added previously.

@monfera Thanks for this very detailed overview.

You've got me convinced, ax._categories can't be sorted from inside the d2c routine.

We need to fill in ax._categories earlier. I think within makeCalcdata makes the most sense at the moment.

Filling in ax._categories within the default step would definitely work (albeit with a slight performance hit).

We already need to loop all traces to check for box plot irregularities here, so maybe you could fill in ax._categories within the same loop.

@etpinard I'll look into it, thanks! Unrelated: I learnt that date axes take ISO strings "2016-03-29" as values. When I expressly indicate axis type = 'category' it behaves as categories and the axis ticks get rendered as plaintext "2016-03-29" rather than nicely formatted date. So I'm not yet grounded in the approach for coercion which was step #2 in your original comment.

@etpinard I'll look into it more closely today, but on an initial look, the suggested place isn't as trivial (to me) and I'd put it slightly after this point, because

setAutoType bails if ax.type!=='-' and at the only place it's called from, there is an identical (redundant) check

the loop in question depends on isBoxWithoutPositionCoords returning true

the inside of the loop (now) is solely dedicated to building up the boxPositions

if axis type 'category' isn't explicitly specified, it won't be known until the subsequent call to autoType() at the end

Therefore I'm planning to put the logic after having returned from setAutoType; the earliest point seems to be just before the call to setConvert.

This way, we wouldn't reuse the suggested loop, but the loop over the traces probably doesn't have a measurable performance impact anyway (I'm assuming there usually aren't thousands or 10ks of trace lines).

I'll do some more tests to ensure I'm not overlooking something; just wanted to share current thinking.

Anywhere, in plots/cartesian/axis_defaults.js is fine at this stage. We'll fine tune the location for performance once the functionality is in a working state.

@etpinard Thanks for the quick note, I'll go ahead.

…scending' at the discussed point

etpinard · 2016-04-05T14:48:26Z

test/jasmine/tests/calcdata_test.js

+                Plotly.plot(gd, [{x: ['c','a','e','b','d'], y: [15,11,12,13,14]}], { xaxis: {
+                    type: 'category',
+                    categorymode: 'trace'
+                    // Wouldn't it be preferred to supply a function and plotly would have several functions like this?


it would be nice 😏 , but only for JS users.

All plotly.js attributes must be JSON serializable so that folks using our python and R libraries can have access to the same features.

…if there's no category encountered for a specific categorylist, it should yield null rather than being skipped over)

…ecked; no assumption about trace order (unlike my first cut of the test cases)

monfera · 2016-04-07T12:42:30Z

test/jasmine/tests/calcdata_test.js

+                expect(gd.calcdata[0][1]).toEqual(jasmine.objectContaining({x: 0, y: 11}));
+                expect(gd.calcdata[0][2]).toEqual(jasmine.objectContaining({x: 4, y: 12}));
+                expect(gd.calcdata[0][3]).toEqual(jasmine.objectContaining({x: 1, y: 13}));
+                expect(gd.calcdata[0][4]).toEqual(jasmine.objectContaining({x: 3, y: 14}));
            });


@etpinard Just an FYI, since I started this CR with the test cases, my conception on where order would be present became outdated. As now I understand that gd.calcdata will keep the trace order, I'm updating test cases such that the original trace order is expected, and explicit checks on the x/y tuples are in place. It looks good to me but please tell me if you have an alternative suggestion.

nicely done.

… axis order checking

…ss trivial places

monfera · 2016-04-09T09:50:00Z

test/jasmine/tests/calcdata_test.js

+                var domTickTexts = Array.prototype.slice.call(document.querySelectorAll('g.xtick'))
+                    .map(function(e) {return e.__data__.text;});
+
+                expect(domTickTexts).toEqual(['b', 'x', 'a', 'd', 'z', 'e', 'c']);  // y, q and k has no data points


@etpinard a few questions regarding this type of test:

I've used library-free querying of the DOM; I think it's good but let me know if I should import d3 for this purpose, or you could point out another test file as a pattern to follow.

I haven't yet bumped into axes/ticks tests of this nature, where we query the DOM, which is downstream of the 'viewModel' testing this file mostly has but upstream of image based testing. Do we need such DOM based testing, and are there preexisting DOM tests I should instead use that ensure e.g. proper category or tracing order?

If we need such tests, should I add similar ones for the other test cases in this file?

…ems to be used across the entire suite)

monfera · 2016-04-12T15:47:07Z

test/jasmine/tests/axes_test.js

-var createGraph = require('../assets/create_graph_div');
-var destroyGraph = require('../assets/destroy_graph_div');
+var createGraphDiv = require('../assets/create_graph_div');
+var destroyGraphDiv = require('../assets/destroy_graph_div');



@etpinard I appended 'Div' to these, to be in sync with how it's usually called.

nicely done.

monfera · 2016-04-12T15:55:41Z

src/plots/cartesian/category_mode_defaults.js

+
+        }
+    }
+};


@etpinard Is this a reasonable cut at the coercion?

…trings (with test cases)

…on sort

monfera · 2016-04-12T22:24:46Z

src/plots/cartesian/ordered_categories.js

-// flattenUniqueSort :: String -> Function -> [[String]] -> [String]
-function flattenUniqueSort(axisLetter, sortFunction, data) {
-    return flattenUnique(axisLetter, data).sort(sortFunction);
+    return categoryArray;
 }



@etpinard I quite rewrote the ordering logic this evening on initial suspicion that image based cases may fail in presence of a large number of points, because of possible slowness due to the O(N) complexity. I switched to bisection. While it didn't make a difference, better scalability is good anyway. As it changed a good bit, it's useful if you know about it.

looks good 👍

Legend item wrap with layout.legend.orientation = h

monfera added 7 commits March 25, 2016 13:11

Reifying current default ordering logic with a passing test case and …

87ab745

…adding new usages with (currently) failing test cases

Ensure that null / undefined removal is in place, even in the presenc…

a547482

…e of order specification

Ensure that no errors arise from the possibility of broader order spe…

69bf832

…c than data coverage

Ensure that it adheres to the categories array even if it doesn't com…

3fd3cc0

…pletely cover the supplied input data

Adding attribute definitions: categorymode and categories; simplifyin…

e2977c5

…g categorymode values

Renaming of 'categories' to 'categorylist' (it was explicitly deleted…

62c4ac5

… presumably as part of providing compatibility with an older API version)

Minimal commit to demonstrate the working of categorymode = 'array'

85d60d6

monfera mentioned this pull request Mar 29, 2016

Control categorical ordering from layout plotly/plotly.js#189

Closed

etpinard reviewed Mar 29, 2016
View reviewed changes

monfera added 4 commits March 31, 2016 13:36

Minimal change for implementing 'category ascending' and 'category de…

f6c40b6

…scending' at the discussed point

factored out orderedCategories into a separate function

e4f2167

factored out orderedCategories into a separate function

03643fe

lint orderedCategories

9c366ce

etpinard reviewed Apr 5, 2016
View reviewed changes

monfera added 3 commits April 7, 2016 13:29

plotly#189 role: info

f626b08

plotly#189 updating preexisting test cases in line with PR feedback (…

2f939af

…if there's no category encountered for a specific categorylist, it should yield null rather than being skipped over)

plotly#189 updating preexisting test cases so that order itself is ch…

ce35e8b

…ecked; no assumption about trace order (unlike my first cut of the test cases)

monfera reviewed Apr 7, 2016
View reviewed changes

monfera added 2 commits April 7, 2016 15:54

plotly#189 updating preexisting test cases: further updates to proper…

262c747

… axis order checking

plotly#189 updating preexisting test cases: further updates to proper…

5f33a7b

… axis order checking

plotly#189 adding actual DOM axis tick order tests for a couple of le…

7c355b8

…ss trivial places

monfera reviewed Apr 9, 2016
View reviewed changes

monfera added 7 commits April 9, 2016 15:46

plotly#189 rewriting cateogory sorter to O(1) + one sort call

8a2292c

plotly#189 rewriting cateogory sorter: extract out logic

9b01bcc

plotly#189 rewriting cateogory sorter: switching to switch

010451b

plotly#189 rewriting cateogory sorter: misc. improvements

6c9d0b5

plotly#189 renaming for uniformity (predominantly createGraphDiv() se…

166aeb4

…ems to be used across the entire suite)

plotly#189 initial round of coercions with test cases

e472e14

plotly#189 additional tests for categorymode coercions

d16fe6b

monfera reviewed Apr 12, 2016
View reviewed changes

plotly#189 comment fix

0314f37

monfera reviewed Apr 12, 2016
View reviewed changes

monfera added 7 commits April 12, 2016 18:27

plotly#189 PR feedback and linting

043ac1b

plotly#189 image based regression test JSONs

b98bd65

plotly#189 reworking the order code because it converted numbers to s…

e007f73

…trings (with test cases)

plotly#189 adding image based tests

34a5054

plotly#189 comment update

c6d44d9

plotly#189 switching from O(n) to O(log(N)) complexity unique inserti…

44a0c3d

…on sort

plotly#189 adding axis attributes to 3D plots

613fda3

monfera reviewed Apr 12, 2016
View reviewed changes

monfera mentioned this pull request Apr 13, 2016

Axis category ordering - adds feature #189 plotly/plotly.js#419

Merged

monfera pushed a commit that referenced this pull request Aug 11, 2016

Merge pull request #1 from psalmody/h-legend-wrap

919d8c8

Legend item wrap with layout.legend.orientation = h

monfera added a commit that referenced this pull request Feb 5, 2017

moving gd[].dimensions out of parcoords.js #1

133f3ab

monfera added a commit that referenced this pull request Feb 12, 2017

moving gd[].dimensions out of parcoords.js #1

eac1f7d

monfera added a commit that referenced this pull request Oct 10, 2017

globally safe clipPath #1

fa84c68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

189 ordinal scale domain item ordering #1

189 ordinal scale domain item ordering #1

monfera commented Mar 29, 2016

etpinard Mar 29, 2016

monfera Mar 29, 2016

monfera Mar 29, 2016

monfera Mar 29, 2016

etpinard Mar 29, 2016

etpinard Mar 29, 2016

monfera Mar 29, 2016

monfera Mar 30, 2016

etpinard Mar 30, 2016

monfera Mar 30, 2016

etpinard Apr 5, 2016

monfera Apr 7, 2016

etpinard Apr 7, 2016

monfera Apr 9, 2016

monfera Apr 12, 2016

etpinard Apr 12, 2016

monfera Apr 12, 2016

etpinard Apr 12, 2016

monfera Apr 12, 2016

etpinard Apr 12, 2016

189 ordinal scale domain item ordering #1

Are you sure you want to change the base?

189 ordinal scale domain item ordering #1

Conversation

monfera commented Mar 29, 2016

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