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
8 changes: 6 additions & 2 deletions src/components/annotations/annotation_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,12 @@ module.exports = function handleAnnotationDefaults(annIn, annOut, fullLayout, op

// put the actual click data to bind to into private attributes
// so we don't have to do this little bit of logic on every hover event
annOut._xclick = (xClick === undefined) ? annOut.x : xClick;
annOut._yclick = (yClick === undefined) ? annOut.y : yClick;
annOut._xclick = (xClick === undefined) ?
annOut.x :
Axes.cleanPosition(xClick, gdMock, annOut.xref);
annOut._yclick = (yClick === undefined) ?
annOut.y :
Axes.cleanPosition(yClick, gdMock, annOut.yref);
}

return annOut;
Expand Down
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);
}
22 changes: 0 additions & 22 deletions src/components/annotations3d/convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

var Lib = require('../../lib');
var Axes = require('../../plots/cartesian/axes');
var attributes = require('./attributes');

module.exports = function convert(scene) {
var fullSceneLayout = scene.fullSceneLayout;
Expand Down Expand Up @@ -61,25 +60,4 @@ function mockAnnAxes(ann, scene) {
ann._ya.l2p = function() {
return 0.5 * (1 - ann.pdata[1] / ann.pdata[3]) * size.h * (domain.y[1] - domain.y[0]);
};

// or do something more similar to 2d
// where Annotations.supplyLayoutDefaults is called after in Plots.doCalcdata
// if category axes are found.
function coerce(attr, dflt) {
return Lib.coerce(ann, ann, attributes, attr, dflt);
}

function coercePosition(axLetter) {
var axName = axLetter + 'axis';

// mock in such way that getFromId grabs correct 3D axis
var gdMock = { _fullLayout: {} };
gdMock._fullLayout[axName] = fullSceneLayout[axName];

return Axes.coercePosition(ann, gdMock, coerce, axLetter, axLetter, 0.5);
}

coercePosition('x');
coercePosition('y');
coercePosition('z');
}
20 changes: 15 additions & 5 deletions src/components/annotations3d/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
'use strict';

var Lib = require('../../lib');
var Axes = require('../../plots/cartesian/axes');
var handleArrayContainerDefaults = require('../../plots/array_container_defaults');
var handleAnnotationCommonDefaults = require('../annotations/common_defaults');
var attributes = require('./attributes');
Expand All @@ -26,16 +27,25 @@ function handleAnnotationDefaults(annIn, annOut, sceneLayout, opts, itemOpts) {
return Lib.coerce(annIn, annOut, attributes, attr, dflt);
}

function coercePosition(axLetter) {
var axName = axLetter + 'axis';

// mock in such way that getFromId grabs correct 3D axis
var gdMock = { _fullLayout: {} };
gdMock._fullLayout[axName] = sceneLayout[axName];

return Axes.coercePosition(annOut, gdMock, coerce, axLetter, axLetter, 0.5);
}


var visible = coerce('visible', !itemOpts.itemIsNotPlainObject);
if(!visible) return annOut;

handleAnnotationCommonDefaults(annIn, annOut, opts.fullLayout, coerce);

// do not use Axes.coercePosition here
// as ax._categories aren't filled in at this stage
coerce('x');
coerce('y');
coerce('z');
coercePosition('x');
coercePosition('y');
coercePosition('z');

// if you have one coordinate you should all three
Lib.noneOrAll(annIn, annOut, ['x', 'y', 'z']);
Expand Down
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.ensureNumber = 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;
};

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

Expand Down
35 changes: 14 additions & 21 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,27 @@ 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 cleanPos, pos;

if(axRef === 'paper' || axRef === 'pixel') {
cleanPos = Lib.ensureNumber;
pos = coerce(attr, dflt);
}
else {
} else {
var 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;
}
cleanPos = ax.cleanPos;
}
// finally make sure we have a number (unless date type already returned a string)
containerOut[attr] = isNumeric(pos) ? Number(pos) : dflt;

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

axes.cleanPosition = function(pos, gd, axRef) {
var cleanPos = (axRef === 'paper' || axRef === 'pixel') ?
Lib.ensureNumber :
axes.getFromId(gd, axRef).cleanPos;

return cleanPos(pos);
};

axes.getDataToCoordFunc = function(gd, trace, target, targetArray) {
Expand Down
45 changes: 25 additions & 20 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 ensureNumber = Lib.ensureNumber;

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 All @@ -165,8 +159,8 @@ module.exports = function setConvert(ax, fullLayout) {
function p2l(px) { return (px - ax._b) / ax._m; }

// conversions among c/l/p are fairly simple - do them together for all axis types
ax.c2l = (ax.type === 'log') ? toLog : num;
ax.l2c = (ax.type === 'log') ? fromLog : num;
ax.c2l = (ax.type === 'log') ? toLog : ensureNumber;
ax.l2c = (ax.type === 'log') ? fromLog : ensureNumber;

ax.l2p = l2p;
ax.p2l = p2l;
Expand All @@ -182,18 +176,20 @@ module.exports = function setConvert(ax, fullLayout) {
if(['linear', '-'].indexOf(ax.type) !== -1) {
// all are data vals, but d and r need cleaning
ax.d2r = ax.r2d = ax.d2c = ax.r2c = ax.d2l = ax.r2l = cleanNumber;
ax.c2d = ax.c2r = ax.l2d = ax.l2r = num;
ax.c2d = ax.c2r = ax.l2d = ax.l2r = ensureNumber;

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

ax.cleanPos = ensureNumber;
}
else if(ax.type === 'log') {
// d and c are data vals, r and l are logged (but d and r need cleaning)
ax.d2r = ax.d2l = function(v, clip) { return toLog(cleanNumber(v), clip); };
ax.r2d = ax.r2c = function(v) { return fromLog(cleanNumber(v)); };

ax.d2c = ax.r2l = cleanNumber;
ax.c2d = ax.l2r = num;
ax.c2d = ax.l2r = ensureNumber;

ax.c2r = toLog;
ax.l2d = fromLog;
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 = ensureNumber;
}
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 = ensureNumber;
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 ensureNumber(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
Loading