fix logic for hiding zero lines when they conflict with axis lines #2936

alexcjohnson · 2018-08-22T12:12:54Z

The logic was quite a bit more complicated than just switching ax -> counterAxis... but seems to work in all cases now.

cc @etpinard

etpinard

The logic was quite a bit more complicated than just switching ax -> counterAxis..

I had the same feeling when looking at #2874 - it's nice to finally look the behavior down in those tests you wrote.

etpinard · 2018-08-22T14:10:21Z

test/jasmine/tests/axes_test.js

+            })
+            .then(function() {
+                // x axis still has a zero line on xy2, and y on x2y
+                // all the others have disappeared now


thanks for the 📚

etpinard · 2018-08-22T14:11:56Z

src/plot_api/subroutines.js

-            extraSubplot = (!ya._anchorAxis || subplot !== xa._mainSubplot);
-            if(extraSubplot && ya.ticks && ya.mirror === 'allticks') {
+            extraSubplot = (!ya._anchorAxis || subplot !== ya._mainSubplot);
+            if(extraSubplot && (ya.mirror === 'allticks' || ya.mirror === 'all')) {


Nice find! Is there a test 🔒 ing this down?

ha, there was a test (the subplot case) locking down the changes but not the parts that remained -> added these in 545ad47

etpinard · 2018-08-22T14:12:49Z

src/plots/cartesian/axes.js

+        }
+    }
+
+    function anyCounterAxLineAtZero(counterAxis, rng) {


Very nice function name.

etpinard · 2018-08-22T14:28:58Z

src/plots/cartesian/axes.js

+            0 : ax._length
+        );
+
+        var counterLetterAxes = axes.list(gd, counterLetter);


It would nice to avoid looping over all axes here.

Can we skip this loop when fullLayout._plots[counterAxis._mainSubplot].overlays is empty, and only check lineNearZero(counterAxis, zeroPosition)?

good call - it was almost that simple. one more relevant test in bcd7c06 and 🐎 in 0f59ca2

Good to hear it was that easy. 💯

etpinard · 2018-08-22T14:38:19Z

src/plots/cartesian/axes.js

-            var rng = Lib.simpleMap(ax.range, ax.r2l),
-                showZl = (rng[0] * rng[1] <= 0) && ax.zeroline &&
+            var rng = Lib.simpleMap(ax.range, ax.r2l);
+            var zlData = {x: 0, id: axid};


No need this do in this PR (opening a new issue would be fine), but we'll need to propagate these changes to large-sploms' regl-based grid line logic:

plotly.js/src/traces/splom/base_plot.js

Lines 171 to 183 in b5f8b23

// just like in Axes.doTicks but without the loop over traces

function showZeroLine(ax) {

var rng = Lib.simpleMap(ax.range, ax.r2l);

var p0 = ax.l2p(0);

return (

ax.zeroline &&

ax._vals && ax._vals.length &&

(rng[0] * rng[1] <= 0) &&

(ax.type === 'linear' || ax.type === '-') &&

((p0 > 1 && p0 < ax._length - 1) || !ax.showline)

);

}

where I didn't just reuse the axes.js logic because I wanted to avoid looping over the traces (to compute that hasBarsOrFill boolean). Thinking about this again, looping over traces isn't really a performance hit for sploms (I can't really imagine a user wanting to plot more 10 splom traces on the same graph).

So it would be worth trying to expose and ♻️ this logic here for large-sploms.

Ah, thanks for pointing that out! #2938

etpinard · 2018-08-22T16:11:27Z

Great! 💃

Thanks for including that perf fix and opening an issue for the splom case. 🎸

fix logic for hiding zero lines when they conflict with axis lines

ef53e31

alexcjohnson added bug something broken status: reviewable labels Aug 22, 2018

etpinard reviewed Aug 22, 2018

View reviewed changes

alexcjohnson added 3 commits August 22, 2018 11:13

lock down equivalence of all/allticks for zeroline visibility

545ad47

add zeroline visibility test for overlaying subplots

bcd7c06

speed up zero line logic - remove axes.list when possible

0f59ca2

alexcjohnson mentioned this pull request Aug 22, 2018

Update zero line logic for sploms #2938

Closed

alexcjohnson merged commit a8dd8dd into master Aug 22, 2018

alexcjohnson deleted the zeroline-fix branch August 22, 2018 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix logic for hiding zero lines when they conflict with axis lines #2936

fix logic for hiding zero lines when they conflict with axis lines #2936

alexcjohnson commented Aug 22, 2018

etpinard left a comment

etpinard Aug 22, 2018

etpinard Aug 22, 2018

alexcjohnson Aug 22, 2018

etpinard Aug 22, 2018

etpinard Aug 22, 2018

alexcjohnson Aug 22, 2018

etpinard Aug 22, 2018

etpinard Aug 22, 2018

alexcjohnson Aug 22, 2018

etpinard commented Aug 22, 2018 •

edited

Loading

	// just like in Axes.doTicks but without the loop over traces
	function showZeroLine(ax) {
	var rng = Lib.simpleMap(ax.range, ax.r2l);
	var p0 = ax.l2p(0);

	return (
	ax.zeroline &&
	ax._vals && ax._vals.length &&
	(rng[0] * rng[1] <= 0) &&
	(ax.type === 'linear' \|\| ax.type === '-') &&
	((p0 > 1 && p0 < ax._length - 1) \|\| !ax.showline)
	);
	}

fix logic for hiding zero lines when they conflict with axis lines #2936

fix logic for hiding zero lines when they conflict with axis lines #2936

Conversation

alexcjohnson commented Aug 22, 2018

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Aug 22, 2018 • edited Loading

etpinard commented Aug 22, 2018 •

edited

Loading