cartesian 2dMap: implement sorting of categorical axes #3827

antoinerg · 2019-05-01T20:13:58Z

Closes #1097 (see below #3827 (comment))

Closes #1117 (see below #3827 (comment))

antoinerg · 2019-05-01T20:21:16Z

Closes #1097:
Codepen before: https://codepen.io/antoinerg/pen/bJyOjX
Codepen after: https://codepen.io/antoinerg/pen/EJzGdv

etpinard · 2019-05-02T01:52:24Z

src/traces/heatmap/clean_2d_array.js

@@ -30,12 +30,23 @@ module.exports = function clean2dArray(zOld, transpose) {
        old2new = function(zOld, i, j) { return zOld[i][j]; };
    }

+    var xMap = function(i) {return i;};
+    var yMap = function(i) {return i;};
+    if(trace && trace.type === 'heatmap') {


Hmm. Why just heatmap?

This was the low-hanging fruit. I guess I should enlarge the scope of this PR and implement sorting of categorical axes for contour, histogram2d and all other traces relying on heatmap's calc routine.

Yes please!

definitely useful for histogram2d! I have qualms from a viz-theory standpoint about sorting an axis of a contour plot: if you're drawing contours, which interpolate between data points, it strongly implies a continuous axis, so you really have no business with categories unless they're some sort of a hack of what's really a continuous axis... and sorting a continuous axis is meaningless.

That said, we allow it in the schema, therefore it's a bug if it doesn't work!

Histogram2d and contour are now working in 6b06516

By allowing all traces (except carpet and contourcarpet), the only test that would fail was in bar_test.js. In it, we would create a contour trace without x and y data. In that case, trace._x and trace._y would be empty and it failed. To fix this, we check whether those arrays are empty and ignore categorical sorting if they are. After all, if the data has no categories attached to it, it's not clear how it should be sorted.

src/traces/heatmap/clean_2d_array.js

src/traces/heatmap/calc.js

src/traces/heatmap/clean_2d_array.js

etpinard · 2019-05-02T19:30:31Z

Thanks very much @antoinerg . This PR is looking good!

etpinard · 2019-05-02T19:36:26Z

Oh wait, codepen https://codepen.io/anon/pen/qqdqWq from #1117 still looks off using your latest commit -> https://codepen.io/etpinard/pen/ROXLbB

etpinard · 2019-05-02T19:38:37Z

... it would be ok to leave #1117 un-addressed in this PR by the way. I hoped to fix two bugs in one go, but it doesn't have to be the case 😄

antoinerg · 2019-05-03T18:40:25Z

@etpinard Although I can get heatmapgl to support categoryorder and categoryarray on first render, it doesn't work on Plotly.relayout (somehow the calc routine is not called again). Maybe this is related to #3833 which I just opened. 🤔

etpinard · 2019-05-03T18:56:36Z

Although I can get heatmapgl to support categoryorder and categoryarray on first render, it doesn't work on Plotly.relayout

That's ok. If you're using heatmapgl, I hope that you have a large data set that isn't categorical, so I have 100% no problem calling this an edge case.

Thanks for opening #3833 !

antoinerg · 2019-05-03T19:52:17Z

@etpinard Ok so in the end, there are minimal changes to the code. Would you like to see more tests?

test/jasmine/tests/histogram2d_test.js

etpinard · 2019-05-06T14:05:48Z

Ok so in the end, there are minimal changes to the code. Would you like to see more tests?

Test coverage look good for this fix. Thanks very much! Although I'm a little worried about that @flaky tag you added.

Now, could you investigate (i.e. no need to fix this now) why your patch here does not resolve #1117 (which I also suspect will need a patch in clean2darray)? Thanks!

antoinerg · 2019-05-07T00:22:04Z

Commit e966030 closes #1117:
Codepen before: https://codepen.io/anon/pen/qqdqWq
Codepen after: https://codepen.io/antoinerg/pen/WBbyyP

antoinerg · 2019-05-07T05:04:04Z

One detail I would like to discuss is whether we really want to support categories that are numerical. Right now, gd._fullLayout.xaxis._categories can be an array of integers. However, whenever findCategories gets called, it is turned into an array of strings by returning a call to Object.keys:

plotly.js/src/plots/cartesian/category_order_defaults.js

Lines 11 to 42 in 03b7e09

    
           function findCategories(ax, opts) { 
        
               var dataAttr = opts.dataAttr || ax._id.charAt(0); 
        
               var lookup = {}; 
        
               var axData; 
        
               var i, j; 
        
               if(opts.axData) { 
        
                   // non-x/y case 
        
                   axData = opts.axData; 
        
               } else { 
        
                   // x/y case 
        
                   axData = []; 
        
                   for(i = 0; i < opts.data.length; i++) { 
        
                       var trace = opts.data[i]; 
        
                       if(trace[dataAttr + 'axis'] === ax._id) { 
        
                           axData.push(trace); 
        
                       } 
        
                   } 
        
               } 
        
               for(i = 0; i < axData.length; i++) { 
        
                   var vals = axData[i][dataAttr]; 
        
                   for(j = 0; j < vals.length; j++) { 
        
                       var v = vals[j]; 
        
                       if(v !== null && v !== undefined) { 
        
                           lookup[v] = 1; 
        
                       } 
        
                   } 
        
               } 
        
               return Object.keys(lookup); 
        
           }

This needs to be adapted if we want to properly support numerical categories.

test/image/mocks/heatmap_shared_categories.json

etpinard · 2019-05-07T14:16:06Z

One detail I would like to discuss is whether we really want to support categories that are numerical.

Yes.

Right now, gd._fullLayout.xaxis._categories can be an array of integers. However, whenever findCategories gets called, it is turned into an array of strings by returning a call to Object.keys

Why is that a big deal? I don't see a problem with turning typeof number items in _categories into strings if that makes things easier.

src/traces/heatmap/clean_2d_array.js

etpinard · 2019-05-07T14:19:31Z

src/traces/heatmap/clean_2d_array.js

+            ax && ax.type === 'category' && trace['_' + ax._id.charAt(0)].length) {
+            var axMapping = [];
+            for(i = 0; i < ax._categories.length; i++) {
+                axMapping.push((trace['_' + ax._id.charAt(0) + 'Map'] || trace[ax._id.charAt(0)]).indexOf(ax._categories[i]));


adding var axLetter = ax._id.charAt(0) above probably wouldn't hurt.

Oh and can we use ax._categoriesMap instead of a (potentially) costly indexOf into ax._categories?

One more thing, grepping for xMap and yMap doesn't yield anything for me. Where do they come from?

EDIT: Nevermind. I found them in convert_column_xyz.js.

Oh and can we use ax._categoriesMap instead of a (potentially) costly indexOf into ax._categories?

Unfortunately, we cannot. For example, in the heatmap_shared_categories mock, _categoriesMap links category Team C with index 2. That index is out of range for the second trace since it only has 2 columns. We really need to check whether each trace has data for a given category (and fill with BADNUM) to fix #1117.

Ok, that's very unfortunate, because

for(i = 0; i < ax._categories.length; i++) { trace[ax._id.charAt(0)]).indexOf(ax._categories[i])) }

can be very costly.

What if instead you made _(x|y)CategoryMap lookup objects?

That is, instead of:

if(ax1 && ax1.type === 'category') { trace['_' + var1Name + 'CategoryMap'] = col1vals.map(function(v) { return ax1._categories[v];}); }

we could have

if(ax1 && ax1.type === 'category') { var lookup = trace['_' + var1Name + 'CategoryMap'] = {}; for(var i = 0; i < col1vals.length; i++) { var v = col1vals[i]; lookup[v] = i; } }

About your questions regarding hasOwnProperty:

we have it in several places already and it seems well supported

hasOwnProperty is roughly 6 times faster than indexOf

However, we now run it for every element so in the end, I'm not sure which is faster with real data.

Sorry @antoinerg I got confused (again). Your logic looks right to me.

The way you have it setup right now, I think we can get rid of the axMapping.hasOwnProperty(ax._categories[i]) and just have:

function(i) { var ind = axMapping[ax._categories[i]]; return ind + 1 ? ind : BADNUM; };

where the + 1 takes care of the ind === 0 case.

This comment is non-blocking as I don't expect much of a performance gain from it 😏

where the + 1 takes care of the ind === 0 case.

Awesome trick :) Done in c7cd1f3

src/traces/heatmap/clean_2d_array.js

…pping

antoinerg · 2019-05-07T17:14:53Z

@etpinard I updated the various Codepens to use the latest version f180c79!

etpinard · 2019-05-07T21:40:50Z

Nice work @antoinerg !

Thanks for commit 0c41776 - which should be a step in the right direction towards cleaning up our categories stashes (we still have a long way to go though).

The best part about this PR are the tests, so let's get in 💃

I'm not sure if GitHub is smart enough to close multiple issues at once, so make sure both #1097 and #1117 are closed off correctly.

antoinerg added 2 commits April 30, 2019 19:22

heatmap: reorder data based on categoryarray

e1716c7

heatmap: 🔒 down categoryorder and categoryarray with tests

d8ef1d7

antoinerg added 2 commits May 1, 2019 16:37

heatmap: handle undefined axes in clean_2d_array

9735683

heatmap: properly sort categorical data when z data is a 1D array

5798dd8

antoinerg added status: reviewable bug something broken and removed status: reviewable labels May 1, 2019

antoinerg added 3 commits May 1, 2019 19:40

heatmap: do not sort categorical axes for contour

56c6a4d

heatmap: only sort categorical axes for trace type heatmap

f878387

heatmap: fix - check trace exists before checking type

a34ca45

etpinard reviewed May 2, 2019

View reviewed changes

heatmap: handle categorical axis sorting except for (contour)carpet

6b06516

etpinard reviewed May 2, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Outdated Show resolved Hide resolved

etpinard reviewed May 2, 2019

View reviewed changes

src/traces/heatmap/calc.js Outdated Show resolved Hide resolved

etpinard reviewed May 2, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Outdated Show resolved Hide resolved

etpinard reviewed May 2, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Outdated Show resolved Hide resolved

heatmap calc: improve style, test contour and heatmap

59edb57

antoinerg added 3 commits May 5, 2019 19:58

🔒 down category ordering in histogram2d(contour) with tests

22cfa82

🔒 down category ordering in heatmapgl with tests

02e4089

test: tag @flaky test, 🔪 useless lines

5160948

etpinard reviewed May 6, 2019

View reviewed changes

test/jasmine/tests/histogram2d_test.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

categorical heatmap: handle missing categories, 1D data and hover

e966030

antoinerg changed the title ~~heatmap: implement sorting of categorical axes~~ cartesian 2dMap: implement sorting of categorical axes May 6, 2019

antoinerg added 2 commits May 6, 2019 19:29

categorical contour: fix jasmine test

ae3ecf5

categorical 2dMap: fill missing category in input data with undefined

e68c170

antoinerg added status: reviewable and removed status: in progress labels May 7, 2019

etpinard reviewed May 7, 2019

View reviewed changes

test/image/mocks/heatmap_shared_categories.json Show resolved Hide resolved

etpinard reviewed May 7, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Outdated Show resolved Hide resolved

etpinard reviewed May 7, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Show resolved Hide resolved

etpinard reviewed May 7, 2019

View reviewed changes

src/traces/heatmap/clean_2d_array.js Show resolved Hide resolved

antoinerg added 4 commits May 7, 2019 11:30

categorial 2dMap: replace undefined with BADNUM

ebf545f

categorical 2dMap: update mock and baseline

793c2a3

categorical 2dmap: fix flaky tests

4d4b0b7

categorical 2dmap: do not use indexOf to find trace-level category ma…

f180c79

…pping

categorical cartesian: convert numbers to strings for _categories

0c41776

categorical 2dMap: remove hasOwnProperty() call

c7cd1f3

antoinerg merged commit 19299af into master May 8, 2019

antoinerg deleted the issue-1097 branch May 8, 2019 01:34

etpinard mentioned this pull request Jul 11, 2019

Fix clean2dArray for categorial x/y with extra items #4038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cartesian 2dMap: implement sorting of categorical axes #3827

cartesian 2dMap: implement sorting of categorical axes #3827

antoinerg commented May 1, 2019 •

edited

Loading

antoinerg commented May 1, 2019 •

edited

Loading

etpinard May 2, 2019

antoinerg May 2, 2019

etpinard May 2, 2019

alexcjohnson May 2, 2019

antoinerg May 2, 2019 •

edited

Loading

etpinard commented May 2, 2019

etpinard commented May 2, 2019

etpinard commented May 2, 2019 •

edited

Loading

antoinerg commented May 3, 2019

etpinard commented May 3, 2019

antoinerg commented May 3, 2019

etpinard commented May 6, 2019 •

edited

Loading

This comment has been minimized.

antoinerg commented May 7, 2019

antoinerg commented May 7, 2019 •

edited

Loading

etpinard commented May 7, 2019 •

edited

Loading

etpinard May 7, 2019

etpinard May 7, 2019

etpinard May 7, 2019 •

edited

Loading

antoinerg May 7, 2019

etpinard May 7, 2019

etpinard May 7, 2019

antoinerg May 7, 2019 •

edited

Loading

etpinard May 7, 2019

antoinerg May 7, 2019

etpinard May 8, 2019

antoinerg commented May 7, 2019

etpinard commented May 7, 2019

cartesian 2dMap: implement sorting of categorical axes #3827

cartesian 2dMap: implement sorting of categorical axes #3827

Conversation

antoinerg commented May 1, 2019 • edited Loading

antoinerg commented May 1, 2019 • 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

antoinerg May 2, 2019 • edited Loading

Choose a reason for hiding this comment

etpinard commented May 2, 2019

etpinard commented May 2, 2019

etpinard commented May 2, 2019 • edited Loading

antoinerg commented May 3, 2019

etpinard commented May 3, 2019

antoinerg commented May 3, 2019

etpinard commented May 6, 2019 • edited Loading

This comment has been minimized.

antoinerg commented May 7, 2019

antoinerg commented May 7, 2019 • edited Loading

etpinard commented May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 7, 2019 • 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

antoinerg May 7, 2019 • 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

antoinerg commented May 7, 2019

etpinard commented May 7, 2019

antoinerg commented May 1, 2019 •

edited

Loading

antoinerg commented May 1, 2019 •

edited

Loading

antoinerg May 2, 2019 •

edited

Loading

etpinard commented May 2, 2019 •

edited

Loading

etpinard commented May 6, 2019 •

edited

Loading

antoinerg commented May 7, 2019 •

edited

Loading

etpinard commented May 7, 2019 •

edited

Loading

etpinard May 7, 2019 •

edited

Loading

antoinerg May 7, 2019 •

edited

Loading