Clean subplots #2227

alexcjohnson · 2017-12-29T06:03:47Z

Updates subplot creation logic. The meat of this is in 3625be4, which makes it so new subplots are created only by traces (visible OR invisible) and components (annotations, shapes, images), but NOT by axis, scene, or subplot objects.

The primary motivation for this is if you make a trace of one type and change it to another, you won't get subplots corresponding to the old type even if we automatically pushed information for the old subplot back into the layout (for example axis ranges). We can leave those objects in the layout without them being rendered, and yet if you come back to the old trace types later any configuration you had used previously will still be available - a nice side effect in editor contexts.

I added a few more _private attributes in the course of this work - which makes pieces a bit more coupled, not so great for testing... but my reasoning is that the logic is fairly complex (we don't want to be redoing it all the time) and nonlocal (the axes/subplots need to know about what's in data and component arrays) so it seemed cleaner to just do it right once during supplyDefaults and reference that forever after.

In order to get this to work right, I altered the svg structure a bit, and simplified some of the rendering logic such as Axes.doTicks a little. This had some happy side effects like removing excess subplot containers and redundant gridlines when you have multiple cartesian subplots (leads to smaller svg files and presumably a bit faster tick/grid rendering - this is why a few mocks in 2de217e got thinner gridlines, they're no longer being overdrawn), and along the way I fixed a couple of other random subtle bugs:

colorbar title-avoids-tick-labels algorithm was incorrect when the top of the colorbar wasn't at the top of the plot area (where it is by default). This is why the airfoil mock changed in 2de217e, that title was avoiding the wrong label. The fix for this one is wrapped up with other logic changes in the big commit 3625be4 - the code in colorbars/draw changed, and now it's fixed, but it's only right in conjunction with other changes in cartesian/axes so I couldn't split it out into a separate commit.
scene.cameraposition -> scene.camera conversion had a typo, fixed in 26aaffc. It turns out a couple of mocks do use cameraposition and changed as a result of this -> 276d1d3

@etpinard please review. Apologies for the giant monolithic commit, fraid I couldn't see a reasonable way to split it up. That probably speaks to the need to refactor this code more than I had the stamina for in this PR...

cc @bpostlethwaite @nicolaskruchten

more efficient, DRY, and without the hack of basePlot.attr='type'

not from axes and scenes. So if you make axes or scenes and then change the trace types, the old ones will not appear.

a few just have different rounding, but most were actually wrong before

so it passes on my mac

alexcjohnson · 2017-12-29T06:09:28Z

dist/translation-keys.txt

@@ -1,12 +1,12 @@
 Autoscale // components/modebar/buttons.js:139
 Box Select // components/modebar/buttons.js:103
-Click to enter Colorscale title // plots/plots.js:437
+Click to enter Colorscale title // plots/plots.js:338


hmm, I should have just ignored the changes to this file, since they're only line number changes... I should revisit this so it doesn't get regenerated just because you run test-syntax locally.

As a rule, I believe we should only commit changes to files in dist/ when running npm version.

alexcjohnson · 2017-12-29T06:24:31Z

src/plot_api/subroutines.js

+ // check whether the relevant position is the same.
+ var sideIndex = alignmentConstants.FROM_BL[side];
+ if(counterAx.side === side) {
+ return anchorAx.domain[sideIndex] === ax.domain[sideIndex];


This happens fairly often in our mocks, and is super easy for users to do: for example make stacked subplots xy and x2y2, then leave y.anchor out because everything looks right even though it defaults to x and not x2. Perhaps we should make better default logic (pick the first non-overlaying axis that will actually make a subplot with this axis, or perhaps the first axis that specifies this axis as its own anchor?) but that logic is kinda tricky and nearly circular. Anyway what this does is as long as the subplot you're drawing looks like the two axes are anchored together, it will join the lines neatly at the corner.

This wasn't needed before because we generated so many extra overlaying subplots, one of them was sure to make a crisp corner...

alexcjohnson · 2017-12-29T06:32:31Z

src/plots/cartesian/axes.js

@@ -1847,7 +1774,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
 if(axLetter === 'x') {
 sides = ['bottom', 'top'];
 transfn = function(d) {
- return 'translate(' + ax.l2p(d.x) + ',0)';
+ return 'translate(' + (ax._offset + ax.l2p(d.x)) + ',0)';


At some point I think we'd be better off just merging ax._offset into ax.*2p and getting rid of all the <g> translations we have scattered about. The fact that it's not in there is a remnant of the days when plots were nested <svg> elements so within a subplot pixel coordinates always started at (0,0). Here at least I'm getting rid of the <g> translations associated with ticks and grids, but it means I have to put them back into the tick and grid paths until *2p catches up.

Hmm. My gut feeling tells me we're better off leaving _offset out of *2p for to make the *2p methods more easily reusable to non-cartesian subplot types that have an offset that isn't just a function of the domain and fullLayout._size (e.g. ternary and polar). But then again perhaps ternary, geo and polar could mock _offset and get the same results. It is something to think about for sure though 🤔

alexcjohnson · 2017-12-29T06:44:50Z

src/plots/cartesian/axes.js

 }
+ drawTicks(mainPlotinfo[axLetter + 'axislayer'], tickpath);


instead of trying to draw all the ticks in a single loop, now I'm splitting it up into the main subplot (the main subplot is the one you're anchored to and may get ticks on one or both sides) and all other subplots (which, if they get any ticks, always get both sides).

This also simplifies figuring out where to put labels, since they always go only on at ax._mainLinePosition and we then no longer need to pay attention here to if that's an anchored or free position or which side of the subplot to pick out.

alexcjohnson · 2017-12-29T06:49:44Z

src/plots/cartesian/axes.js

+
+ // keep track of which subplots (by main conteraxis) we've already
+ // drawn grids for, so we don't overdraw overlaying subplots
+ var finishedGrids = {};


note that you can't do something simple like pick the main axis for each overlaying subplot, as that subplot may not exist (say for example you have x2y2 overlaying xy - for y2 the main counteraxis is x but there is no xy2 subplot. So instead I just make sure we only draw the grid once for any given main axis.

alexcjohnson · 2017-12-29T06:53:21Z

src/plots/cartesian/axis_ids.js

@@ -65,7 +64,7 @@ function listNames(gd, axLetter, only2d) {
 var names = filterAxis(fullLayout, '');
 if(only2d) return names;

- var sceneIds3D = Plots.getSubplotIds(fullLayout, 'gl3d') || [];


removed one circular dep here (only 16 to go 🙄 ) by having _subplots already calculated. I removed Plots.getSubplotIds entirely since it had become a 1-liner just looking up entries in fullLayout._subplots.

Beautiful cc #236

alexcjohnson · 2017-12-29T07:08:04Z

src/plots/plots.js

+ // I kind of think we should just let all traces increment color, visible or not.
+ // see mock: axes-autotype-empty vs. a test of restyling visible: false that
+ // I can't find right now...
+ if(fullTrace._input.visible !== false) colorCnt++;


@etpinard thoughts? Should visible: false traces still be allowed to increment color? I guess one reason to permit disqualified traces to increment color is in case they become valid later on (like by streaming the first data point into one) but I feel like that argument applies to explicitly visible: false traces too, ie showing one would not be expected to change all the other colors. But I guess technically that would be a breaking change so maybe should wait for v2? As I have it here we preserve backward compatibility.

You make a great point about streaming data. So yeah, I agree: visible: false traces should be allowed to increment color in v2. Let's add that to #420 🚬

done in #420 (comment)

alexcjohnson · 2017-12-29T07:21:59Z

test/jasmine/tests/toimage_test.js

@@ -149,7 +149,7 @@ describe('Plotly.toImage', function() {
 .then(function() { return Plotly.toImage(gd, {format: 'png', imageDataOnly: true}); })
 .then(function(d) {
 expect(d.indexOf('data:image/')).toBe(-1);
- expect(d.length).toBeWithin(53660, 1e3, 'png image length');
+ expect(d.length).toBeWithin(54660, 3e3, 'png image length');


This had been failing on my computer for ages, though it always works on CI... now there's just one test, in gl3d code, that consistently fails for me, though I need to be careful to have the chrome window on my secondary monitor; if I let it live on my laptop screen (retina) a whole bunch of other tests fail too. Must be a devicePixelRatio dependence or something. Would love to sort this out at some point, makes it annoying to run the full test suite if I'm not at home, and god forbid I ever upgrade this monitor and only have high-res devices to work with!

etpinard

A very nice display of perseverance in this PR 💪

If I were you, I probably would've given up three times by now 😉

This thing is looking really solid. Above all, I'm a big fan of fullLayout._subplots and includeBasePlot. In this first review, I made several comments, mostly questions, none of which feel major.

etpinard · 2017-12-29T13:56:57Z

src/plots/plots.js

- *
- * @return {array} array of calcdata traces
- */
-plots.getSubplotCalcData = function(calcData, type, subplotId) {


I'm curious. Why did you move this to a separate file? Does leaving it in plots/plots.js lead to a circular dependency?

Why did you move this to a separate file?

I don't recall if it made more circular deps or not, but I don't like plots/plots - it has its fingers in everything. Plus I was making 2 (3 as of the next commit) similar simple functions for filtering down data or calcdata that only depend on Registry so it seemed natural to group them in their own file.

etpinard · 2017-12-29T13:58:36Z

src/plots/cartesian/layout_defaults.js

- xaListCheater = [],
- xaListNonCheater = [],
+ xaCheater = {},
+ xaNonCheater = {},


etpinard · 2017-12-29T14:00:26Z

dist/translation-keys.txt

@@ -1,12 +1,12 @@
 Autoscale // components/modebar/buttons.js:139
 Box Select // components/modebar/buttons.js:103
-Click to enter Colorscale title // plots/plots.js:437
+Click to enter Colorscale title // plots/plots.js:338


As a rule, I believe we should only commit changes to files in dist/ when running npm version.

etpinard · 2017-12-29T14:03:14Z

src/components/colorbar/draw.js

@@ -433,7 +436,7 @@ module.exports = function draw(gd, id) {
 selection: d3.select(gd).selectAll('g.' + cbAxisOut._id + 'tick'),
 side: opts.titleside,
 offsetLeft: gs.l,
- offsetTop: gs.t,
+ offsetTop: 0,


Does this fix #970 ?

or maybe #1396

I don't think it fixes #970 but yes, it does fix #1396 🎉

etpinard · 2017-12-29T14:05:31Z

src/lib/index.js

+ * alphanumeric string sort, tailored for subplot IDs like scene2, scene10, x10y13 etc
+ */
+var char0 = 48;
+var char9 = 57;


brilliant ✨

etpinard · 2017-12-29T15:15:49Z

test/jasmine/tests/annotations_test.js

- xref: 'x5', // will be converted to 'x' and xaxis should autorange
- yref: 'y5', // same 'y' -> yaxis
+ xref: 'xq', // will be converted to 'x' and xaxis should autorange
+ yref: 'yz', // same 'y' -> yaxis


Why did you change this?

Why did you change this?

Now an annotation is enough to create a new subplot, so I had to switch these to invalid references.

etpinard · 2017-12-29T15:24:44Z

test/jasmine/tests/gl_plot_interact_test.js

- expect(gd.layout.xaxis).toBeUndefined();
- expect(gd.layout.yaxis).toBeUndefined();
+ expect(gd.layout.xaxis === undefined).toBe(true);
+ expect(gd.layout.yaxis === undefined).toBe(true);


Something is off with toBeUndefined?

when you expect(somePotentiallyBigObject).toBeUndefined(), a failure swamps you with the whole stringified object. The killer was expect(scene) whose string representation was so big it took me forever to find the tracebacks. If the object you might get on failure is small, toBeUndefined is still a good idea as it shows you what you got.

etpinard · 2017-12-29T15:28:57Z

test/jasmine/tests/plots_test.js

@@ -758,4 +671,134 @@ describe('Test Plots', function() {
 });
 });
 });
+
+ describe('subplot cleaning logic', function() {


Lovely tests

etpinard · 2017-12-29T15:31:51Z

test/jasmine/tests/transform_groupby_test.js

@@ -9,6 +9,15 @@ var customAssertions = require('../assets/custom_assertions');
 var assertDims = customAssertions.assertDims;
 var assertStyle = customAssertions.assertStyle;

+
+function supplyDataDefaults(dataIn, dataOut) {
+ return Plots.supplyDataDefaults(dataIn, dataOut, {}, {


It's getting more difficult to call anything but Plots.supplyDefaults in tests due to the large amount of things to mocks. We should maybe start standardising around assets/supply_defaults.js instead?

right, that's the disadvantage of this kind of precalculation, which I've added a lot of lately 🙈

👍 for moving toward assets/supply_defaults.js.

etpinard · 2017-12-29T15:39:19Z

src/plots/plots.js

+ }
+
+ // ensure all cartesian axes have at least one subplot
+ if(layoutOut._has('cartesian') || layoutOut._has('gl2d')) {


Why do we need the || layoutOut._has('gl2d') part here?

In fact we don't! Good 👀
We depend on gl2d inside finalizeSubplots but it's irrelevant if we have only gl2d. 6e62b35

etpinard · 2017-12-29T16:05:27Z

src/plots/cartesian/include_components.js

+ var idRegex = Cartesian.idRegex;
+ var subplots = layoutOut._subplots;
+ var xaList = subplots.xaxis;
+ var yaList = subplots.yaxis;


There's a few more Axes.list that could be replaced by _subplots.xaxis and _subplots.yaxis

Ah right - but actually listIds is the direct match to fullLayout._subplots.(x|y)axis (with some _subplots.gl3d shenanigans thrown in for good measure), as list returns the axis objects. I'll take a stab at removing listIds and simplifying the others.

Cleaned up list and listIds in 0506f88 and 2aa1545

in particular, annotations autorange was including 3d axes, and axes we already knew didn't have annotations.

need to filter out (x|y)ref='paper'

etpinard · 2018-01-03T16:41:00Z

A well-earned 💃

I'll take care of #2227 (comment) while trying to get the tests to pass on the regl branch.

- add 'expandIndex' for all traces - add 'group' for traces with enable groupby transforms - 🔪 itemNumber (which was undefined on double click, can add it back in future if someone asks for it.

alexcjohnson added 10 commits December 22, 2017 10:33

refactor calcdata filtering

687a2e2

more efficient, DRY, and without the hack of basePlot.attr='type'

perf xaCheater/xaNonCheater test in cartesian axes defaults

9a18062

lint cartesian layout_defaults

2c9d90f

rework subplot logic - create subplots from traces and components only

3625be4

not from axes and scenes. So if you make axes or scenes and then change the trace types, the old ones will not appear.

update baselines for improved subplot logic

2de217e

a few just have different rounding, but most were actually wrong before

wider acceptance range for png toimage test

b94ad8e

so it passes on my mac

update trans keys

c88e8c2

fix typo in cameraposition -> camera cleaning

26aaffc

updated gl3d "cameraposition" baselines

276d1d3

🔪 unused import

c49007e

alexcjohnson commented Dec 29, 2017

View reviewed changes

etpinard reviewed Dec 29, 2017

View reviewed changes

etpinard added status: reviewable bug something broken labels Dec 29, 2017

etpinard added this to the v1.32.0 milestone Dec 29, 2017

etpinard reviewed Dec 29, 2017

View reviewed changes

alexcjohnson mentioned this pull request Dec 29, 2017

wishlist for potential breaking changes since v1 #420

Closed

15 tasks

alexcjohnson added 5 commits December 29, 2017 21:07

🔪 commented out code

698561c

Object.keys -> for in, in plots/plots

8804311

simplify redundant title removal

4786b6e

get rid of unneeded _has(gl2d)

6e62b35

use _subplots in Axes.list/listIds

0506f88

alexcjohnson added 2 commits December 30, 2017 02:59

simplify and speed up a few Axes.list uses

2aa1545

in particular, annotations autorange was including 3d axes, and axes we already knew didn't have annotations.

fix annotation calc_autorange change

44060db

need to filter out (x|y)ref='paper'

alexcjohnson merged commit 3a4788b into master Jan 3, 2018

alexcjohnson deleted the clean-subplots branch January 3, 2018 17:07

This was referenced Jan 3, 2018

Colorbar tick labels overlap with colorbar title #1396

Closed

don't write out strings from find_locale_strings during tests #2232

Merged

etpinard mentioned this pull request Jan 8, 2018

Remaining subplot problems #291

Closed

5 tasks

This was referenced Jan 25, 2018

Gridlines duplication after adding and removing trace #1760

Closed

Plotly#redraw (1.20.4.min.js) appears to not clean up properly #1218

Closed

Bug: Titles not being shown #2301

Closed

etpinard mentioned this pull request Feb 2, 2018

Mixed up axes assignments in subplots #1312

Closed

alexcjohnson mentioned this pull request Feb 6, 2018

Plotly.react #2341

Merged

vinipitta mentioned this pull request Feb 7, 2018

subplot duplication in overlayed axis #2347

Closed

igorpejic mentioned this pull request Feb 26, 2018

Axes not redrawing after layout change plotly/react-plotly.js#45

Closed

etpinard mentioned this pull request Mar 7, 2018

Fix rangeslider titles with stacked y axes #2451

Merged

alexcjohnson mentioned this pull request Mar 7, 2018

scatter trace fails to disappear on legend hide #2452

Closed

etpinard mentioned this pull request Apr 19, 2018

Speed up cartesian axes supplyDefaults #2549

Closed

Clean subplots #2227

Clean subplots #2227

Conversation

alexcjohnson commented Dec 29, 2017

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

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 left a comment • edited Loading

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

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

Choose a reason for hiding this comment

etpinard commented Jan 3, 2018 • edited Loading

etpinard left a comment •

edited

Loading

etpinard commented Jan 3, 2018 •

edited

Loading