Skip to content

Fixup rangebreaks l2p and p2l functions #4699

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

Merged
merged 14 commits into from
Mar 29, 2020
Merged

Fixup rangebreaks l2p and p2l functions #4699

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8df7b6d
correct p2l for rangebreaks
archmoj Mar 28, 2020
4c5d245
ensure min < max and fix hover on reversed ranges
archmoj Mar 28, 2020
4c66949
add failing test - wrong mapping on reversed ranges
archmoj Mar 28, 2020
11c8ea2
fix rangebreaks mapping on reversed ranges - issue 4700
archmoj Mar 28, 2020
063632d
check for Infinity in rangebreak l2p
archmoj Mar 29, 2020
077b34f
make tick loop readable
archmoj Mar 29, 2020
50a2681
optimize fns a bit - pass 1
archmoj Mar 29, 2020
5876608
optimize fns more - pass 2
archmoj Mar 29, 2020
2ed2ec8
fixup test for m2
archmoj Mar 29, 2020
7511cb1
avoid reverse array twice
archmoj Mar 29, 2020
ce2e206
apply only one sign
archmoj Mar 29, 2020
18099ae
reduce if block
archmoj Mar 29, 2020
749430d
fix autorange reversed
archmoj Mar 29, 2020
ff5db2b
add new hover tests
archmoj Mar 29, 2020
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
6 changes: 4 additions & 2 deletions src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,8 +636,10 @@ axes.calcTicks = function calcTicks(ax) {
var newTickVals = [];
var prevPos;

var signAx = axrev ? -1 : 1;
for(var q = axrev ? 0 : len - 1; signAx * q >= signAx * (axrev ? len - 1 : 0); q -= signAx) { // apply reverse loop to pick greater values in breaks first
var dir = axrev ? 1 : -1;
var first = axrev ? 0 : len - 1;
var last = axrev ? len - 1 : 0;
for(var q = first; dir * q <= dir * last; q += dir) { // apply reverse loop to pick greater values in breaks first
var pos = ax.c2p(tickVals[q].value);

if(prevPos === undefined || Math.abs(pos - prevPos) > tf2) {
Expand Down
61 changes: 33 additions & 28 deletions src/plots/cartesian/set_convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,51 +198,55 @@ module.exports = function setConvert(ax, fullLayout) {
if(!len) return _l2p(v, ax._m, ax._b);

var isY = axLetter === 'y';
var pos = isY ? -v : v;
var pos = v;

var q = 0;
for(var i = 0; i < len; i++) {
var flip = isY;
if(ax.range[0] > ax.range[1]) flip = !flip;
var signAx = flip ? -1 : 1;

var first = 0;
var last = len - 1;
var q = first;
for(var i = first; i <= last; i += 1) {
var nextI = i + 1;
var brk = ax._rangebreaks[i];

var min = isY ? -brk.max : brk.min;
var max = isY ? -brk.min : brk.max;
var min = brk.min;
var max = brk.max;

if(pos < min) break;
if(pos > max) q = nextI;
if(signAx * pos < signAx * min) break;
if(signAx * pos > signAx * max) q = nextI;
else {
// when falls into break, pick 'closest' offset
q = pos > (min + max) / 2 ? nextI : i;
q = signAx * pos > signAx * (min + max) / 2 ? nextI : i;
Copy link
Collaborator

Choose a reason for hiding this comment

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

l2p is a hot path, so it's worth optimizing a good deal - even if it means some duplicated code, though I'm not sure if that would help here, and even if the code needs more comments and is less self-documenting. For example:

  • signAx * pos could be done once up front, and var min = signAx * brk.min etc would simplify these conditionals a bit
  • first and last look unnecessary
  • Flip _m2 for y axes after using it to calculate _B, so we don't need the (isY ? -1 : 1) * down below.
  • Move var isY = axLetter === 'y' to the outer scope, where axLetter was defined.

p2l I think is generally not as hot, though maybe for some traces it's used in hover? Not sure. (Note that c2p uses l2p and p2c uses p2l)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good calls. Addressed in the commits below.

break;
}
}
return _l2p(v, (isY ? -1 : 1) * ax._m2, ax._B[q]);
var b2 = ax._B[q] || 0;
if(!isFinite(b2)) return 0; // avoid NaN translate e.g. in positionLabels if one keep zooming exactly into a break

return _l2p(v, (isY ? -1 : 1) * ax._m2, b2);
};

p2l = function(px) {
if(!isNumeric(px)) return BADNUM;
var len = ax._rangebreaks.length;
if(!len) return _p2l(px, ax._m, ax._b);

var isY = axLetter === 'y';
var pos = isY ? -px : px;
var pos = px;

var q = 0;
for(var i = 0; i < len; i++) {
var first = 0;
var last = len - 1;
var q = first;
for(var i = first; i <= last; i += 1) {
var nextI = i + 1;
var brk = ax._rangebreaks[i];

var min = isY ? -brk.pmax : brk.pmin;
var max = isY ? -brk.pmin : brk.pmax;

if(pos < min) break;
if(pos > max) q = nextI;
else {
q = i;
break;
}
if(pos < brk.pmin) break;
if(pos > brk.pmax) q = nextI;
}
return _p2l(px, (isY ? -1 : 1) * ax._m2, ax._B[q]);
var b2 = ax._B[q];
return _p2l(px, (isY ? -1 : 1) * ax._m2, b2);
};
}

Expand Down Expand Up @@ -588,20 +592,21 @@ module.exports = function setConvert(ax, fullLayout) {
ax._B.push(-ax._m2 * rl0);
}

if(axReverse) {
ax._rangebreaks.reverse();
}

for(i = 0; i < ax._rangebreaks.length; i++) {
brk = ax._rangebreaks[i];
ax._B.push(ax._B[ax._B.length - 1] - ax._m2 * (brk.max - brk.min) * signAx);
}
if(axReverse) {
ax._B.reverse();
}

// fill pixel (i.e. 'p') min/max here,
// to not have to loop through the _rangebreaks twice during `p2l`
for(i = 0; i < ax._rangebreaks.length; i++) {
brk = ax._rangebreaks[i];
brk.pmin = l2p(axReverse ? brk.max : brk.min);
brk.pmax = l2p(axReverse ? brk.min : brk.max);
brk.pmin = l2p(brk.min);
brk.pmax = l2p(brk.max);
}
}
}
Expand Down
Binary file modified test/image/baselines/axes_breaks-night_autorange-reversed.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/axes_breaks-reversed-without-pattern.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.
237 changes: 237 additions & 0 deletions test/image/mocks/axes_breaks-reversed-without-pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
{
"data": [
{
"type": "scatter",
"mode": "markers+lines",
"x": [
"1970-01-01 00:00:00.000",
"1970-01-01 00:00:00.010",
"1970-01-01 00:00:00.020",
"1970-01-01 00:00:00.030",
"1970-01-01 00:00:00.040",
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.060",
"1970-01-01 00:00:00.070",
"1970-01-01 00:00:00.080",
"1970-01-01 00:00:00.090",
"1970-01-01 00:00:00.100",
"1970-01-01 00:00:00.110",
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.130",
"1970-01-01 00:00:00.140",
"1970-01-01 00:00:00.150",
"1970-01-01 00:00:00.160",
"1970-01-01 00:00:00.170",
"1970-01-01 00:00:00.180",
"1970-01-01 00:00:00.190",
"1970-01-01 00:00:00.200"
]
},
{
"xaxis": "x2",
"yaxis": "y2",
"type": "scatter",
"mode": "markers+lines",
"x": [
"1970-01-01 00:00:00.000",
"1970-01-01 00:00:00.010",
"1970-01-01 00:00:00.020",
"1970-01-01 00:00:00.030",
"1970-01-01 00:00:00.040",
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.060",
"1970-01-01 00:00:00.070",
"1970-01-01 00:00:00.080",
"1970-01-01 00:00:00.090",
"1970-01-01 00:00:00.100",
"1970-01-01 00:00:00.110",
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.130",
"1970-01-01 00:00:00.140",
"1970-01-01 00:00:00.150",
"1970-01-01 00:00:00.160",
"1970-01-01 00:00:00.170",
"1970-01-01 00:00:00.180",
"1970-01-01 00:00:00.190",
"1970-01-01 00:00:00.200"
]
},
{
"xaxis": "x3",
"yaxis": "y3",
"type": "scatter",
"mode": "markers+lines",
"orientation": "h",
"y": [
"1970-01-01 00:00:00.000",
"1970-01-01 00:00:00.010",
"1970-01-01 00:00:00.020",
"1970-01-01 00:00:00.030",
"1970-01-01 00:00:00.040",
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.060",
"1970-01-01 00:00:00.070",
"1970-01-01 00:00:00.080",
"1970-01-01 00:00:00.090",
"1970-01-01 00:00:00.100",
"1970-01-01 00:00:00.110",
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.130",
"1970-01-01 00:00:00.140",
"1970-01-01 00:00:00.150",
"1970-01-01 00:00:00.160",
"1970-01-01 00:00:00.170",
"1970-01-01 00:00:00.180",
"1970-01-01 00:00:00.190",
"1970-01-01 00:00:00.200"
]
},
{
"xaxis": "x4",
"yaxis": "y4",
"type": "scatter",
"mode": "markers+lines",
"orientation": "h",
"y": [
"1970-01-01 00:00:00.000",
"1970-01-01 00:00:00.010",
"1970-01-01 00:00:00.020",
"1970-01-01 00:00:00.030",
"1970-01-01 00:00:00.040",
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.060",
"1970-01-01 00:00:00.070",
"1970-01-01 00:00:00.080",
"1970-01-01 00:00:00.090",
"1970-01-01 00:00:00.100",
"1970-01-01 00:00:00.110",
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.130",
"1970-01-01 00:00:00.140",
"1970-01-01 00:00:00.150",
"1970-01-01 00:00:00.160",
"1970-01-01 00:00:00.170",
"1970-01-01 00:00:00.180",
"1970-01-01 00:00:00.190",
"1970-01-01 00:00:00.200"
]
}
],
"layout": {
"showlegend": false,
"width": 800,
"height": 800,
"xaxis": {
"rangebreaks": [
{
"bounds": [
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.075"
]
},
{
"bounds": [
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.180"
]
}
],
"domain": [
0,
0.48
]
},
"xaxis2": {
"rangebreaks": [
{
"bounds": [
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.075"
]
},
{
"bounds": [
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.180"
]
}
],
"autorange": "reversed",
"anchor": "y2",
"domain": [
0.52,
1
]
},
"xaxis3": {
"anchor": "y3",
"domain": [
0,
0.48
]
},
"xaxis4": {
"anchor": "y4",
"domain": [
0.52,
1
]
},
"yaxis": {
"domain": [
0,
0.48
]
},
"yaxis2": {
"anchor": "x2",
"domain": [
0.52,
1
]
},
"yaxis3": {
"rangebreaks": [
{
"bounds": [
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.075"
]
},
{
"bounds": [
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.180"
]
}
],
"anchor": "x3",
"domain": [
0.52,
1
]
},
"yaxis4": {
"rangebreaks": [
{
"bounds": [
"1970-01-01 00:00:00.050",
"1970-01-01 00:00:00.075"
]
},
{
"bounds": [
"1970-01-01 00:00:00.120",
"1970-01-01 00:00:00.180"
]
}
],
"autorange": "reversed",
"anchor": "x4",
"domain": [
0,
0.48
]
}
}
}