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

Jump to bottom

number like categories respect layout.autotypenumbers #5880

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

number like categories respect layout.autotypenumbers #5880

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9caf482
Simply trying to force all strings to be categories
nicholas-esterer Aug 3, 2021
536e6e9
Added notes for clarification needs in the tests
nicholas-esterer Aug 5, 2021
5fc0ade
possibly break on call to getRangePositionForCategory
nicholas-esterer Aug 6, 2021
69ea57c
Numeric categories in SVG paths intepreted as indices
nicholas-esterer Aug 9, 2021
bf90246
Original shapes test now passing
nicholas-esterer Aug 9, 2021
b60fe18
linting
nicholas-esterer Aug 10, 2021
6b47a05
Merge branch 'master' into number-like-categories-misinterpret
nicholas-esterer Aug 10, 2021
89cbf11
we cannot cast numeric strings to numbers in getDataToPixel
nicholas-esterer Aug 10, 2021
b3896a2
Simplification of dataToPixel code
nicholas-esterer Aug 10, 2021
8ac107a
have to make special case for category axis in shapes test
nicholas-esterer Aug 11, 2021
536e513
numeric-string-categories mock, shapes_test linting
nicholas-esterer Aug 11, 2021
0b48de5
Added numeric-string-categories baseline image
nicholas-esterer Aug 12, 2021
e9901c2
Added numeric categories tests to shapes_test
nicholas-esterer Aug 13, 2021
74631d8
Merge branch 'master' into number-like-categories-misinterpret
nicholas-esterer Aug 16, 2021
e8277a9
removed fdescribe (!)
nicholas-esterer Aug 16, 2021
b53aa49
Added baselines for images and annotations
nicholas-esterer Aug 16, 2021
9128f4c
mock json linting
nicholas-esterer Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/shapes/calc_autorange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ function shapeBounds(ax, v0, v1, path, paramsToUse) {
var val;

if(ax.type === 'date') convertVal = helpers.decodeDate(convertVal);
if(ax.type === 'category') convertVal = helpers.castNumericStringsToNumbers(convertVal);

for(i = 0; i < segments.length; i++) {
segment = segments[i];
Expand Down
78 changes: 40 additions & 38 deletions src/components/shapes/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,27 +383,44 @@ function setupDragElement(gd, shapePath, shapeOptions, index, shapeLayer, editHe
removeVisualCues(shapeLayer);
}

function moveShape(dx, dy) {
if(shapeOptions.type === 'path') {
var noOp = function(coord) { return coord; };
var moveX = noOp;
var moveY = noOp;
// Compute moveX or moveY, ax is xa or ya, p2xy is p2x or p2y, xy2p is x2p
// or y2p, dxy is dx or dy
function computeMove(ax, p2xy, xy2p, dxy) {
var move = function(xy) { return p2xy(xy2p(xy) + dxy); };
if(ax && ax.type === 'date') {
move = helpers.encodeDate(move);
}
if(ax && ax.type === 'category') {
move = function(xy) {
return p2xy(helpers.castNumericStringsToNumbers(xy2p)(xy) + dxy);
};
}
return move;
}

if(xPixelSized) {
modifyItem('xanchor', shapeOptions.xanchor = p2x(xAnchor + dx));
} else {
moveX = function moveX(x) { return p2x(x2p(x) + dx); };
if(xa && xa.type === 'date') moveX = helpers.encodeDate(moveX);
}
function doPathMoveOrResize(dx, dy) {
var noOp = function(coord) { return coord; };
var moveX = noOp;
var moveY = noOp;

if(yPixelSized) {
modifyItem('yanchor', shapeOptions.yanchor = p2y(yAnchor + dy));
} else {
moveY = function moveY(y) { return p2y(y2p(y) + dy); };
if(ya && ya.type === 'date') moveY = helpers.encodeDate(moveY);
}
if(xPixelSized) {
modifyItem('xanchor', shapeOptions.xanchor = p2x(xAnchor + dx));
} else {
moveX = computeMove(xa, p2x, x2p, dx);
}

if(yPixelSized) {
modifyItem('yanchor', shapeOptions.yanchor = p2y(yAnchor + dy));
} else {
moveY = computeMove(ya, p2y, y2p, dy);
}

modifyItem('path', shapeOptions.path = movePath(pathIn, moveX, moveY));
}

modifyItem('path', shapeOptions.path = movePath(pathIn, moveX, moveY));
function moveShape(dx, dy) {
if(shapeOptions.type === 'path') {
doPathMoveOrResize(dx, dy);
} else {
if(xPixelSized) {
modifyItem('xanchor', shapeOptions.xanchor = p2x(xAnchor + dx));
Expand All @@ -426,26 +443,7 @@ function setupDragElement(gd, shapePath, shapeOptions, index, shapeLayer, editHe

function resizeShape(dx, dy) {
if(isPath) {
// TODO: implement path resize, don't forget to update dragMode code
var noOp = function(coord) { return coord; };
var moveX = noOp;
var moveY = noOp;

if(xPixelSized) {
modifyItem('xanchor', shapeOptions.xanchor = p2x(xAnchor + dx));
} else {
moveX = function moveX(x) { return p2x(x2p(x) + dx); };
if(xa && xa.type === 'date') moveX = helpers.encodeDate(moveX);
}

if(yPixelSized) {
modifyItem('yanchor', shapeOptions.yanchor = p2y(yAnchor + dy));
} else {
moveY = function moveY(y) { return p2y(y2p(y) + dy); };
if(ya && ya.type === 'date') moveY = helpers.encodeDate(moveY);
}

modifyItem('path', shapeOptions.path = movePath(pathIn, moveX, moveY));
doPathMoveOrResize(dx, dy);
} else if(isLine) {
if(dragMode === 'resize-over-start-point') {
var newX0 = x0 + dx;
Expand Down Expand Up @@ -613,6 +611,10 @@ function getPathString(gd, options) {
if(type === 'path') {
if(xa && xa.type === 'date') x2p = helpers.decodeDate(x2p);
if(ya && ya.type === 'date') y2p = helpers.decodeDate(y2p);
// the SVG path is always a string, so for categories numeric strings
// are always converted to numbers when seen in a SVG path
if(xa && xa.type === 'category') x2p = helpers.castNumericStringsToNumbers(x2p);
if(ya && ya.type === 'category') y2p = helpers.castNumericStringsToNumbers(y2p);
return convertPath(options, x2p, y2p);
}

Expand Down
8 changes: 8 additions & 0 deletions src/components/shapes/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ var constants = require('./constants');

var Lib = require('../../lib');

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

// special position conversion functions... category axis positions can't be
// specified by their data values, because they don't make a continuous mapping.
// so these have to be specified in terms of the category serial numbers,
Expand Down Expand Up @@ -32,6 +34,12 @@ exports.encodeDate = function(convertToDate) {
return function(v) { return convertToDate(v).replace(' ', '_'); };
};

exports.castNumericStringsToNumbers = function(convertToPx) {
return function(v) {
return convertToPx(isNumeric(v) ? Number(v) : v);
};
};

exports.extractPathCoords = function(path, paramsToUse) {
var extractedCoordinates = [];

Expand Down
1 change: 1 addition & 0 deletions src/plots/cartesian/axis_autotype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var isDateTime = Lib.isDateTime;
var cleanNumber = Lib.cleanNumber;
var round = Math.round;

// Determines a type for an axis based on the contents of array.
module.exports = function autoType(array, calendar, opts) {
var a = array;

Expand Down
9 changes: 5 additions & 4 deletions src/plots/cartesian/set_convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var Lib = require('../../lib');
var numberFormat = Lib.numberFormat;
var isNumeric = require('fast-isnumeric');


var cleanNumber = Lib.cleanNumber;
var ms2DateTime = Lib.ms2DateTime;
var dateTime2ms = Lib.dateTime2ms;
Expand Down Expand Up @@ -178,8 +179,8 @@ module.exports = function setConvert(ax, fullLayout) {
if(isNumeric(v)) return +v;
}

function getRangePosition(v) {
return isNumeric(v) ? +v : getCategoryIndex(v);
function getRangePositionForCategory(v) {
return (typeof v !== 'string') ? v : getCategoryIndex(v);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate why using isNumeric here is not possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isNumeric returns true for isNumeric('1') which we don't want. We want numeric string categories to have to use the indices.

}

// include 2 fractional digits on pixel, for PDF zooming etc
Expand Down Expand Up @@ -313,12 +314,12 @@ module.exports = function setConvert(ax, fullLayout) {
ax.d2r = ax.d2l_noadd = getCategoryPosition;

ax.r2c = function(v) {
var index = getRangePosition(v);
var index = getRangePositionForCategory(v);
return index !== undefined ? index : ax.fraction2r(0.5);
};

ax.l2r = ax.c2r = ensureNumber;
ax.r2l = getRangePosition;
ax.r2l = getRangePositionForCategory;

ax.d2p = function(v) { return ax.l2p(ax.r2c(v)); };
ax.p2d = function(px) { return getCategoryName(p2l(px)); };
Expand Down
Binary file added test/image/baselines/annotations-with-numeric-string-categories.png
View file
Open in desktop
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/images-with-numeric-string-categories.png
View file
Open in desktop
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/shapes-with-numeric-string-categories.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
57 changes: 57 additions & 0 deletions test/image/mocks/annotations-with-numeric-string-categories.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
{
"data":[{
"x": ["3", "1", "5", "2", "4", "6"],
"y": ["6","7","8","2","3","4"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we use numbers here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this example, it looks the same, but if you try using numbers in the original issue, you get
bugstillthere

which means the coordinates in a shape refer to the index of the category on the axis if the shape coordinates are a number and the referenced axis is categorical. In this case the axes are categorical but the categories are numbers (see how they are spaced evenly on the axis even though the numbers are not evenly spaced)
This is probably surprising to users, but the most surprising thing, which is reported in the referenced issue, is that a numeric string is interpreted as an index into the axis.

For context, this is the code:

<!DOCTYPE html>
<html>
<head>
    <meta http-equiv='Content-Type' content='text/html:charset=utf-8' />
    <script src="../../build/plotly.js"></script>
</head>
<body>
<div id="tester"></div>
<script>
    var layout = {
      "xaxis": {
        "type": "category",
        "categoryarray": [
          10, 25, 30, 40, 50, 60, 70
        ],
      },
      "shapes": [
        {
          "type": "rect",
          "xref": "x",
          "yref": "y",
          "x0": 40,
          "x1": 60,
          "y0": 10,
          "y1": 20,
          "fillcolor": "rgba(45,180,135,0.3)",
        }
      ]
    };

    var data = [
      {
        "x": [ 10, 30, 50, 70],
        "y": [ 10, 20, 30, 40],
      },
    ];
    
    TESTER = document.getElementById('tester');
    Plotly.newPlot( TESTER, data, layout );
</script>
</body>

</html>

}],
"layout":{
"xaxis": {
"type": "category",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's adjust formatting for these new mocks.

"categoryarray": ["1", "2", "3", "4", "5", "6"]
},
"yaxis": {
"type": "category"
},
"annotations": [
{
"text": "A",
"x": "2",
"y": 3.5
},
{
"text": "B",
"x": 0.5,
"y": "3"
},
{
"text": "C",
"x": 2,
"y": "0"
},
{
"text": "D",
"x": "7",
"y": 3
}
],
"shapes": [
{
"type": "line",
"x0": 0.5,
"x1": 0.5,
"y0": 0,
"y1": 1,
"xref": "x domain",
"yref": "y domain"
},
{
"type": "line",
"y0": 0.5,
"y1": 0.5,
"x0": 0,
"x1": 1,
"xref": "x domain",
"yref": "y domain"
}
]
}
}
86 changes: 86 additions & 0 deletions test/image/mocks/images-with-numeric-string-categories.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
{
"data":[{
"x": ["3", "1", "5", "2", "4", "6"],
"y": ["6","7","8","2","3","4"]
}],
"layout":{
"xaxis": {
"type": "category",
"categoryarray": ["1", "2", "3", "4", "5", "6"]
},
"yaxis": {
"type": "category"
},
"images": [
{
"source": "https://images.plot.ly/language-icons/api-home/js-logo.png",
"xref": "x",
"yref": "y",
"sizex": 1,
"sizey": 1,
"x": "2",
"y": 3.5,
"xanchor": "left",
"yanchor": "top",
"opacity": 0.4
},
{
"source": "https://images.plot.ly/language-icons/api-home/js-logo.png",
"xref": "x",
"yref": "y",
"sizex": 1,
"sizey": 1,
"x": 0.5,
"y": "3",
"xanchor": "left",
"yanchor": "top",
"opacity": 0.6
},
{
"source": "https://images.plot.ly/language-icons/api-home/js-logo.png",
"xref": "x",
"yref": "y",
"sizex": 1,
"sizey": 1,
"x": 2,
"y": "0",
"xanchor": "left",
"yanchor": "top",
"opacity": 0.8

},
{"source": "https://images.plot.ly/language-icons/api-home/js-logo.png",
"xref": "x",
"yref": "y",
"sizex": 1,
"sizey": 1,
"x": "7",
"y": 3,
"xanchor": "left",
"yanchor": "top",
"opacity": 1

}
],
"shapes": [
{
"type": "line",
"x0": 0.5,
"x1": 0.5,
"y0": 0,
"y1": 1,
"xref": "x domain",
"yref": "y domain"
},
{
"type": "line",
"y0": 0.5,
"y1": 0.5,
"x0": 0,
"x1": 1,
"xref": "x domain",
"yref": "y domain"
}
]
}
}
72 changes: 72 additions & 0 deletions test/image/mocks/shapes-with-numeric-string-categories.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
{
"data":[{
"x": ["3", "1", "5", "2", "4", "6"],
"y": ["6","7","8","2","3","4"]
}],
"layout":{
"xaxis": {
"type": "category",
"categoryarray": ["1", "2", "3", "4", "5", "6"]
},
"yaxis": {
"type": "category"
},
"shapes": [
{
"type": "rect",
"x0": "2",
"x1": "4",
"y0": 3.5,
"y1": 4.5,
"line": {"color": "blue"}
},
{
"type": "rect",
"x0": 0.5,
"x1": 1.5,
"y0": "3",
"y1": "4",
"line": {"color": "red"}
},
{
"type": "rect",
"x0": 2,
"x1": 4,
"y0": "0",
"y1": "4",
"line": {"color": "green"}
},
{
"type": "rect",
"x0": "4",
"x1": "7",
"y0": 3,
"y1": 4,
"line": {"color": "yellow"}
},
{
"type": "path",
"path": "M0,5.5L3,4.5L1,3Z",
"line": {"color": "orange"}
},
{
"type": "line",
"x0": 0.5,
"x1": 0.5,
"y0": 0,
"y1": 1,
"xref": "x domain",
"yref": "y domain"
},
{
"type": "line",
"y0": 0.5,
"y1": 0.5,
"x0": 0,
"x1": 1,
"xref": "x domain",
"yref": "y domain"
}
]
}
}