Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cartesian 2dMap: implement sorting of categorical axes #3827

Merged
merged 21 commits into from
May 8, 2019
Merged

cartesian 2dMap: implement sorting of categorical axes #3827

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e1716c7
heatmap: reorder data based on categoryarray
antoinerg Apr 30, 2019
d8ef1d7
heatmap: :lock: down categoryorder and categoryarray with tests
antoinerg May 1, 2019
9735683
heatmap: handle undefined axes in clean_2d_array
antoinerg May 1, 2019
5798dd8
heatmap: properly sort categorical data when z data is a 1D array
antoinerg May 1, 2019
56c6a4d
heatmap: do not sort categorical axes for contour
antoinerg May 1, 2019
f878387
heatmap: only sort categorical axes for trace type heatmap
antoinerg May 2, 2019
a34ca45
heatmap: fix - check trace exists before checking type
antoinerg May 2, 2019
6b06516
heatmap: handle categorical axis sorting except for (contour)carpet
antoinerg May 2, 2019
59edb57
heatmap calc: improve style, test contour and heatmap
antoinerg May 3, 2019
22cfa82
:lock: down category ordering in histogram2d(contour) with tests
antoinerg May 5, 2019
02e4089
:lock: down category ordering in heatmapgl with tests
antoinerg May 6, 2019
5160948
test: tag @flaky test, :hocho: useless lines
antoinerg May 6, 2019
e966030
categorical heatmap: handle missing categories, 1D data and hover
antoinerg May 6, 2019
ae3ecf5
categorical contour: fix jasmine test
antoinerg May 6, 2019
e68c170
categorical 2dMap: fill missing category in input data with undefined
antoinerg May 7, 2019
ebf545f
categorial 2dMap: replace undefined with BADNUM
antoinerg May 7, 2019
793c2a3
categorical 2dMap: update mock and baseline
antoinerg May 7, 2019
4d4b0b7
categorical 2dmap: fix flaky tests
antoinerg May 7, 2019
f180c79
categorical 2dmap: do not use indexOf to find trace-level category ma…
antoinerg May 7, 2019
0c41776
categorical cartesian: convert numbers to strings for _categories
antoinerg May 7, 2019
c7cd1f3
categorical 2dMap: remove hasOwnProperty() call
antoinerg May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/traces/heatmap/clean_2d_array.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,29 @@ module.exports = function clean2dArray(zOld, trace, xa, ya) {
old2new = function(zOld, i, j) { return zOld[i][j]; };
}

var xMap = Lib.identity;
var yMap = Lib.identity;
if(trace && trace.type !== 'carpet' && trace.type !== 'contourcarpet') {
if(ya && ya.type === 'category' && trace._y.length) {
yMap = function(i) {return trace._y[i];};
}
if(xa && xa.type === 'category' && trace._x.length) {
xMap = function(i) {return trace._x[i];};
function axisMapping(ax) {
if(trace && trace.type !== 'carpet' && trace.type !== 'contourcarpet' &&
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]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@etpinard etpinard May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
       }
    }

Copy link
Contributor Author

@antoinerg antoinerg May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Awesome trick :) Done in c7cd1f3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 💃

}
console.log('axMapping', axMapping);
return function(i) {return axMapping[i] === -1 ? undefined : axMapping[i];};
} else {
return Lib.identity;
}
}

var xMap = axisMapping(xa);
var yMap = axisMapping(ya);

var zNew = new Array(rowlen);

if(ya && ya.type === 'category') rowlen = ya._categories.length;
for(i = 0; i < rowlen; i++) {
collen = getCollen(zOld, i);
if(xa && xa.type === 'category') collen = xa._categories.length;
zNew[i] = new Array(collen);
for(j = 0; j < collen; j++) zNew[i][j] = cleanZvalue(old2new(zOld, yMap(i), xMap(j)));
}
Expand Down
8 changes: 8 additions & 0 deletions src/traces/heatmap/convert_column_xyz.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,12 @@ module.exports = function convertColumnData(trace, ax1, ax2, var1Name, var2Name,
}
if(hasColumnText) trace._text = text;
if(hasColumnHoverText) trace._hovertext = hovertext;

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

if(ax2 && ax2.type === 'category') {
trace['_' + var2Name + 'Map'] = col2vals.map(function(v) { return ax2._categories[v];});
}
};
4 changes: 4 additions & 0 deletions src/traces/heatmap/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, hoverLay
} else {
xl = xc ? xc[nx] : ((x[nx] + x[nx + 1]) / 2);
yl = yc ? yc[ny] : ((y[ny] + y[ny + 1]) / 2);

if(xa && xa.type === 'category') xl = x[nx];
if(ya && ya.type === 'category') yl = y[ny];

if(trace.zsmooth) {
x0 = x1 = xa.c2p(xl);
y0 = y1 = ya.c2p(yl);
Expand Down
Binary file added test/image/baselines/heatmap_columnar.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/image/baselines/heatmap_shared_categories.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
13 changes: 13 additions & 0 deletions test/image/mocks/heatmap_columnar.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"data": [{
"type": "heatmap",
"x": ["a", "a", "a", "b", "b", "b", "c", "c", "c"],
"y": ["A", "B", "C", "A", "B", "C", "A", "B", "C"],
"z": [0, 50, 100, 50, 0, 255, 100, 510, 1010]
}],
"layout": {
"xaxis": {
"categoryorder": "category descending"
}
}
}
49 changes: 49 additions & 0 deletions test/image/mocks/heatmap_shared_categories.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
"data": [{
"type": "heatmap",
"x": ["Team A", "Team B", "Team C"],
"xaxis": "x",
"y": ["Game Three", "Game Two", "Game One"],
"z": [
[0.1, 0.3, 0.5],
[1, 0.8, 0.6],
[0.6, 0.4, 0.2]
],
"yaxis": "y"
}, {
"type": "heatmap",
"x": ["Team B", "Team C"],
"xaxis": "x",
"y": ["Game Three", "Game Two", "Game One"],
"z": [
[0.3, 0.5],
[0.8, 0.6],
[0.4, 0.2]
],
"yaxis": "y2"
}],
"layout": {
"xaxis": {
"anchor": "y2",
"domain": [0, 1],
"type": "category",
"range": [-0.5, 2.5],
"autorange": true
},
"yaxis": {
"anchor": "free",
"domain": [0.575, 1],
"position": 0,
"type": "category",
"range": [-0.5, 2.5],
"autorange": true
},
"yaxis2": {
"anchor": "x",
"domain": [0, 0.425],
"type": "category",
"range": [-0.5, 2.5],
"autorange": true
}
}
}
20 changes: 19 additions & 1 deletion test/jasmine/tests/heatmap_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ describe('heatmap calc', function() {

expect(out._xcategories).toEqual(layout.xaxis.categoryarray, 'xaxis should reorder');
expect(out._ycategories).toEqual(layout.yaxis.categoryarray, 'yaxis should reorder');
expect(out.z[0][0]).toEqual(65);
expect(out.z[0][0]).toEqual(0);
});
});
});
Expand Down Expand Up @@ -781,6 +781,24 @@ describe('heatmap hover', function() {
});
});

describe('with sorted categories', function() {
beforeAll(function(done) {
gd = createGraphDiv();

var mock = require('@mocks/heatmap_categoryorder.json');
var mockCopy = Lib.extendDeep({}, mock);

Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done);
});
afterAll(destroyGraphDiv);

it('should find closest point (case 1) and should', function() {
var pt = _hover(gd, 3, 1)[0];
expect(pt.index).toEqual([1, 3], 'have correct index');
assertLabels(pt, 2.5, 0.5, 0);
});
});

describe('for xyz-column traces', function() {
beforeAll(function(done) {
gd = createGraphDiv();
Expand Down