Skip to content

Improved category axes conversions #1748

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.

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

Merged
merged 8 commits into from
May 31, 2017
19 changes: 14 additions & 5 deletions src/components/annotations/click.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,22 @@ function getToggleSets(gd, hoverData) {
explicitOffSet = [],
hoverLen = (hoverData || []).length;

var i, j, anni, showMode, pointj, toggleType;
var i, j, anni, showMode, pointj, xa, ya, toggleType;

for(i = 0; i < annotations.length; i++) {
anni = annotations[i];
showMode = anni.clicktoshow;

if(showMode) {
for(j = 0; j < hoverLen; j++) {
pointj = hoverData[j];
if(pointj.xaxis._id === anni.xref &&
pointj.yaxis._id === anni.yref &&
pointj.xaxis.d2r(pointj.x) === anni._xclick &&
pointj.yaxis.d2r(pointj.y) === anni._yclick
xa = pointj.xaxis;
ya = pointj.yaxis;

if(xa._id === anni.xref &&
ya._id === anni.yref &&
xa.d2r(pointj.x) === clickData2r(anni._xclick, xa) &&
ya.d2r(pointj.y) === clickData2r(anni._yclick, ya)
) {
// match! toggle this annotation
// regardless of its clicktoshow mode
Expand All @@ -121,3 +125,8 @@ function getToggleSets(gd, hoverData) {

return {on: onSet, off: offSet, explicitOff: explicitOffSet};
}

// to handle log axes until v2
function clickData2r(d, ax) {
return ax.type === 'log' ? ax.l2r(d) : ax.d2r(d);
}
12 changes: 12 additions & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
'use strict';

var d3 = require('d3');
var isNumeric = require('fast-isnumeric');

var numConstants = require('../constants/numerical');
var FP_SAFE = numConstants.FP_SAFE;
var BADNUM = numConstants.BADNUM;

var lib = module.exports = {};

Expand Down Expand Up @@ -87,6 +92,13 @@ lib.pushUnique = require('./push_unique');

lib.cleanNumber = require('./clean_number');

lib.num = function num(v) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want a more descriptive name for this now that it's not just internal to one module? ensureNumber? safeNumber?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with ensureNumber in 071249f

if(!isNumeric(v)) return BADNUM;
v = Number(v);
if(v < -FP_SAFE || v > FP_SAFE) return BADNUM;
return isNumeric(v) ? Number(v) : BADNUM;
};

lib.noop = require('./noop');
lib.identity = require('./identity');

Expand Down
30 changes: 8 additions & 22 deletions src/plots/cartesian/axes.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ var ONEDAY = constants.ONEDAY;
var ONEHOUR = constants.ONEHOUR;
var ONEMIN = constants.ONEMIN;
var ONESEC = constants.ONESEC;
var BADNUM = constants.BADNUM;

var axes = module.exports = {};

Expand Down Expand Up @@ -100,33 +99,20 @@ axes.coerceRef = function(containerIn, containerOut, gd, attr, dflt, extraOption
* - for other types: coerce them to numbers
*/
axes.coercePosition = function(containerOut, gd, coerce, axRef, attr, dflt) {
var pos,
newPos;
var ax, pos;

if(axRef === 'paper' || axRef === 'pixel') {
ax = { cleanPos: Lib.num };
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't need to pretend we're making an axis anymore... just cleanPos = Lib.num or cleanPos = ax.cleanPos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 071249f

pos = coerce(attr, dflt);
}
else {
var ax = axes.getFromId(gd, axRef);

} else {
ax = axes.getFromId(gd, axRef);
dflt = ax.fraction2r(dflt);
pos = coerce(attr, dflt);

if(ax.type === 'category') {
// if position is given as a category name, convert it to a number
if(typeof pos === 'string' && (ax._categories || []).length) {
newPos = ax._categories.indexOf(pos);
containerOut[attr] = (newPos === -1) ? dflt : newPos;
return;
}
}
else if(ax.type === 'date') {
containerOut[attr] = Lib.cleanDate(pos, BADNUM, ax.calendar);
return;
}
}
// finally make sure we have a number (unless date type already returned a string)
containerOut[attr] = isNumeric(pos) ? Number(pos) : dflt;

containerOut[attr] = ax.cleanPos(pos);
};

};

axes.getDataToCoordFunc = function(gd, trace, target, targetArray) {
Expand Down
37 changes: 21 additions & 16 deletions src/plots/cartesian/set_convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var Lib = require('../../lib');
var cleanNumber = Lib.cleanNumber;
var ms2DateTime = Lib.ms2DateTime;
var dateTime2ms = Lib.dateTime2ms;
var num = Lib.num;

var numConstants = require('../../constants/numerical');
var FP_SAFE = numConstants.FP_SAFE;
Expand All @@ -28,13 +29,6 @@ function fromLog(v) {
return Math.pow(10, v);
}

function num(v) {
if(!isNumeric(v)) return BADNUM;
v = Number(v);
if(v < -FP_SAFE || v > FP_SAFE) return BADNUM;
return isNumeric(v) ? Number(v) : BADNUM;
}

/**
* Define the conversion functions for an axis data is used in 5 ways:
*
Expand Down Expand Up @@ -152,7 +146,7 @@ module.exports = function setConvert(ax, fullLayout) {
if(index !== undefined) return index;
}

if(typeof v === 'number') { return v; }
if(isNumeric(v)) return +v;
}

function l2p(v) {
Expand Down Expand Up @@ -186,6 +180,8 @@ module.exports = function setConvert(ax, fullLayout) {

ax.d2p = ax.r2p = function(v) { return ax.l2p(cleanNumber(v)); };
ax.p2d = ax.p2r = p2l;

ax.cleanPos = num;
}
else if(ax.type === 'log') {
// d and c are data vals, r and l are logged (but d and r need cleaning)
Expand All @@ -203,6 +199,8 @@ module.exports = function setConvert(ax, fullLayout) {

ax.r2p = function(v) { return ax.l2p(cleanNumber(v)); };
ax.p2r = p2l;

ax.cleanPos = num;
}
else if(ax.type === 'date') {
// r and d are date strings, l and c are ms
Expand All @@ -222,24 +220,31 @@ module.exports = function setConvert(ax, fullLayout) {

ax.d2p = ax.r2p = function(v, _, calendar) { return ax.l2p(dt2ms(v, 0, calendar)); };
ax.p2d = ax.p2r = function(px, r, calendar) { return ms2dt(p2l(px), r, calendar); };

ax.cleanPos = function(v) { return Lib.cleanDate(v, BADNUM, ax.calendar); };
}
else if(ax.type === 'category') {
// d is categories; r, c, and l are indices
// TODO: should r accept category names too?
// ie r2c and r2l would be getCategoryIndex (and r2p would change)
// d is categories (string)
// c and l are indices (numbers)
// r is categories or numbers

ax.d2r = ax.d2c = ax.d2l = setCategoryIndex;
ax.d2c = ax.d2l = setCategoryIndex;
ax.r2d = ax.c2d = ax.l2d = getCategoryName;

// special d2l variant that won't add categories
ax.d2l_noadd = getCategoryIndex;
ax.d2r = ax.d2l_noadd = getCategoryIndex;

ax.r2l = ax.l2r = ax.r2c = ax.c2r = num;
ax.l2r = ax.r2c = ax.c2r = num;
ax.r2l = getCategoryIndex;

ax.d2p = function(v) { return ax.l2p(getCategoryIndex(v)); };
ax.p2d = function(px) { return getCategoryName(p2l(px)); };
ax.r2p = ax.l2p;
ax.r2p = ax.d2p;
ax.p2r = p2l;

ax.cleanPos = function(v) {
if(typeof v === 'string' && v !== '') return v;
return num(v);
};
}

// find the range value at the specified (linear) fraction of the axis
Expand Down
24 changes: 3 additions & 21 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2032,7 +2032,7 @@ plots.doCalcdata = function(gd, traces) {
}
}

var hasCategoryAxis = initCategories(axList);
initCategories(axList);

var hasCalcTransform = false;

Expand Down Expand Up @@ -2101,25 +2101,11 @@ plots.doCalcdata = function(gd, traces) {
}

Registry.getComponentMethod('fx', 'calc')(gd);

// To handle the case of components using category names as coordinates, we
// need to re-supply defaults for these objects now, after calc has
// finished populating the category mappings
// Any component that uses `Axes.coercePosition` falls into this category
if(hasCategoryAxis) {
var dataReferencedComponents = ['annotations', 'shapes', 'images'];
for(i = 0; i < dataReferencedComponents.length; i++) {
Registry.getComponentMethod(dataReferencedComponents[i], 'supplyLayoutDefaults')(
gd.layout, fullLayout, fullData);
}
}
};

// initialize the category list, if there is one, so we start over
// to be filled in later by ax.d2c
function initCategories(axList) {
var hasCategoryAxis = false;

// initialize the category list, if there is one, so we start over
// to be filled in later by ax.d2c
for(var i = 0; i < axList.length; i++) {
axList[i]._categories = axList[i]._initialCategories.slice();

Expand All @@ -2128,11 +2114,7 @@ function initCategories(axList) {
for(var j = 0; j < axList[i]._categories.length; j++) {
axList[i]._categoriesMap[axList[i]._categories[j]] = j;
}

if(axList[i].type === 'category') hasCategoryAxis = true;
}

return hasCategoryAxis;
}

plots.rehover = function(gd) {
Expand Down
11 changes: 8 additions & 3 deletions src/traces/heatmap/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,15 @@ module.exports = function calc(gd, trace) {
z = binned.z;
}
else {
if(hasColumns(trace)) convertColumnData(trace, xa, ya, 'x', 'y', ['z']);
if(hasColumns(trace)) {
convertColumnData(trace, xa, ya, 'x', 'y', ['z']);
x = trace.x;
y = trace.y;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting... were we double-converting triplet data on category and date axes before? Do we have a test of these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

were we double-converting triplet

Right. Previously ax.d2c was called twice in heatmap/calc.js. For example, on master the heatmap_xyz-dates-and-categories mock has:

gd._fullLayout.yaxis._categories
// ["3-month", "6-month", "1-year", "2-year", "3-year", "5-year", 0, 1, 2, 3, 4, 5]

which still gives the correct result though as the string categories are ignored during r2p.

Now that, starting in this PR, r2p convert strings to pixels, this patch is now necessary to make the heatmap_xyz-dates-and-categories pass.

Do we have a test of these cases?

Good call. I added heatmap/calc test case to 🔒 this down more explicitly in 4434fad

} else {
x = trace.x ? xa.makeCalcdata(trace, 'x') : [];
y = trace.y ? ya.makeCalcdata(trace, 'y') : [];
}

x = trace.x ? xa.makeCalcdata(trace, 'x') : [];
y = trace.y ? ya.makeCalcdata(trace, 'y') : [];
x0 = trace.x0 || 0;
dx = trace.dx || 1;
y0 = trace.y0 || 0;
Expand Down
23 changes: 0 additions & 23 deletions test/jasmine/tests/annotations_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,39 +98,16 @@ describe('Test annotations', function() {
expect(layoutOut.annotations[0].ax).toEqual('2004-07-01');
});

it('should convert ax/ay category coordinates to linear coords', function() {
var layoutIn = {
annotations: [{
showarrow: true,
axref: 'x',
ayref: 'y',
x: 'c',
ax: 1,
y: 'A',
ay: 3
}]
};

var layoutOut = {
xaxis: {
type: 'category',
_categories: ['a', 'b', 'c'],
range: [-0.5, 2.5] },
yaxis: {
type: 'category',
_categories: ['A', 'B', 'C'],
range: [-0.5, 3]
}
};
Axes.setConvert(layoutOut.xaxis);
Axes.setConvert(layoutOut.yaxis);

_supply(layoutIn, layoutOut);

expect(layoutOut.annotations[0].x).toEqual(2);
expect(layoutOut.annotations[0].ax).toEqual(1);
expect(layoutOut.annotations[0].y).toEqual(0);
expect(layoutOut.annotations[0].ay).toEqual(3);
});
});
});
Expand Down
Loading