Layout grids #2399

alexcjohnson · 2018-02-23T17:05:30Z

An easy way to make a grid of subplots: layout.grid can arrange cartesian subplots either as coupled layouts (x axes span entire columns and y axes span rows) or independent (each grid cell gets a specified x/y subplot; axes are coupled only as specified by the subplots). Other types of subplot (and non-subplot traces that support domain) can be placed in the grid too, by specifying domain: {row, column}

Created to support SPLOM #2372 (comment), but hopefully its usefulness will be more general than that.

alexcjohnson · 2018-02-23T17:06:21Z

test/image/mocks/grouped_bar.json

@@ -33,11 +33,6 @@
    "xaxis": {
      "type": "category"
    },
-    "barmode": "group",
-    "categories": [


maybe this was supposed to be xaxis.categories? Anyway it was ignored so 🔪

This mock is ancient. Way older than the axis.categories attribute. This must be a typo. 👍

alexcjohnson · 2018-02-23T17:07:57Z

test/jasmine/tests/gl3d_plot_interact_test.js

-        // 200 was not robust for me (AJ), 300 seems to be.
-        return delay(300)();
+        // 300 was not robust for me (AJ), 500 seems to be.
+        return delay(500)();


Should I be worried that I've had to increase this delay twice now? I just ignored this test for a while, but it had been failing for me locally for at least the last 6 months...

alexcjohnson · 2018-02-23T17:14:32Z

src/lib/coerce.js

-        otherOpts: ['dflt', 'freeLength'],
+        // set dimensions=2 for a 2D array
+        // `items` may be a single object instead of an array, in which case
+        // `freeLength` must be true.


I needed 1D and 2D arbitrary-length info_array for specifying axes and/or subplots for the grid. I didn't actually end up using coerce with these, since there's validation involved where allowed values depend on both the available axes/subplots and on earlier elements in the same array; but it's still important for documentation, and for Plotly.react it's important that these are marked as info_array and not data_array.

Anyway, they work, in case we find a need in the future... tested below.

Good stuff!

Would you mind updating the updatemenus and sliders attributes

so that they use items: {} instead of the not-as-general items: [{}, {}, {}].

These ones are for function arguments, and we know that the functions being called have a maximum of 3 arguments. The [{}, {}, {}] form ensures we never go past three args. I suppose it probably wouldn't hurt to change it, but I didn't feel like taking that risk as I'm not sure how well tested the edge cases are for these components.

Good point here about args being at-most a three-item array. Let's keep that as it 👌

But this has me thinking: should we change attributes like ticktext and tickvals to free-length info_array instead of their current data_array val type? That way, we could get rid of this ugly hack.

should we change attributes like ticktext and tickvals to free-length info_array instead of their current data_array val type?

Hmm, possibly... would certainly be nice to keep data_array out of layout, but those are potentially long enough (and likely enough to be connected to a data source) to still warrant being called data_array so I'm a bit ambivalent about it. We could discuss this in a separate issue.

We could discuss this in a separate issue.

Sounds good 👍

Let's continue that discussion in -> #1894

alexcjohnson · 2018-02-23T17:20:03Z

src/lib/coerce.js

+
+                exports.valObjectMeta[opts.valType].coerceFunction(v, propPart, dflt, opts);
+
+                return out;


I had change this section to support non-array items - but also for existing info_array attributes this should be substantially faster than using regular coerce for each of the elements, since we're not constructing fake attribute strings and using them in the multiple nestedProperty constructs that coerce does.

Amazing thanks!

alexcjohnson · 2018-02-23T17:28:42Z

src/plots/grid.js

+        editType: 'calc',
+        description: [
+            'Is the first row the top or the bottom? Note that columns',
+            'are always enumerated from left to right.'


until someone from a RTL country asks I guess? 😅

alexcjohnson · 2018-02-23T17:44:29Z

src/plots/grid.js

+            '*xy* or *x3y2*, or ** to leave that cell empty. You may reuse x axes',
+            'within the same column, and y axes within the same row.',
+            'Non-cartesian subplots and traces that support `domain` can place themselves',
+            'in this grid separately using the `gridcell` attribute.'


There would be something nice about specifying non-cartesian subplots in this array as well... but there are two reasons I didn't allow that:

It's easy to specify 'scene2' and 'geo3' etc but how do you specify a trace (like pie)? You'd have to use uid or something. I feel like unless we can get everything, we should only allow cartesian subplots this way.

What if you use the xaxes and yaxes form (or one of the defaults with pattern) but you still want to insert something else? Like, say you have a splom but you want to put pies along the diagonal, or a splom showing only above the diagonal and you want to put some unrelated subplots below the diagonal?

Also I should mention it might even be useful to allow cartesian axes to be inserted in the grid separately, like with a row or column attribute that plucks the appropriate domain from the grid. Like if you make a splom (using xaxes and yaxes) and want something special along the diagonal (box plots for example) - these may want to share a y axis with the grid but have a separate x axis (categorical in that case). We might be able to do this with overlaying but that might not work because the axis it's overlaying doesn't actually have a subplot on the diagonal... anyway that seems a kind of hacky solution, would be cleaner to just specify the column.

but how do you specify a trace (like pie)?

we could probably do pie (i.e. first pie trace in gd.data), pie2 (2nd pie trace in gd.data), pie3 and similar for other traces that provide their own subplot. But yeah, it's not ideal.

What if you use the xaxes and yaxes form (or one of the defaults with pattern) but you still want to insert something else? Like, say you have a splom but you want to put pies along the diagonal, or a splom showing only above the diagonal and you want to put some unrelated subplots below the diagonal?

Could we allow setting multiple grids per graph? e.g. grid, grid2, grid3 that would overlay one another?

... and also allowing multiple grids could be a nice way to generate grids with multi-colspan, multi-rowspan cells and crazy shared axes pattern.

I think multiple overlaying grids is likely to be more confusing than helpful. colspan/rowspan would be easier to understand and use as just that - a way to specify colspan and rowspan within the existing grid, some of them wouldn't even work as a different grid, unless you monkey with grid.domain, which would be super confusing, and getting the gaps to line up correctly would be automatic with colspan/rowspan but nontrivial between grids having a different number of rows and columns.

I suppose for my other use case, inserting extra subplots in an otherwise fully coupled layout, we could support providing xaxes, yaxes, and subplots.

we could probably do pie (i.e. first pie trace in gd.data), pie2 (2nd pie trace in gd.data), pie3 and similar for other traces that provide their own subplot. But yeah, it's not ideal.

Yeah, my concern with that is order in gd.data can be changed, or you can delete or change the type of an earlier trace, and then you'd have to know to update the reference in the grid. Also it's ambiguous, does pie2 mean the pie in trace 2 or the second pie?

Anyway, it's pretty low-footprint, if we do think of a nice way to do this I don't think it would be a problem to add it and continue supporting the way I have it now.

alexcjohnson · 2018-02-23T17:47:58Z

src/plots/grid.js

+            'then iterating rows according to `roworder`.'
+        ].join(' ')
+    },
+    gap: {


I used xgap and ygap to match heatmap, but I felt like it's also useful to have a symmetric gap attribute (that's only used to set the defaults for xgap and ygap) - thoughts? Keep this (and add it to heatmap), or remove it and just use xgap/ygap?

Also, for completeness, in other contexts I've seen either grout or gutter used for this effect, do we want to stick with gap or use one of these?

I'd vote to 🔪 gap and stick to xgap and ygap. The python api make_subplots only uses horizontal_spacing and vertical_spacing with different default values:

Moreover, I prefer gap over gutter and grout (which is a word I never heard before 📚 ), but this isn't a strong opinion.

Sounds good, I'll 🔪 gap and keep xgap and ygap.

The meaning of horizontal/vertical spacing in the python wrapper is different from what I have for xgap/ygap - the python version seems to use absolute domain fraction (hence the need to divide the default by the number of cols/rows, whereas in my current implementation it's a fraction of the grid period, ie xgap = xgapPx / (xgapPx + xplotPx). That allows me to use a constant value for the default, and I feel like it's easier to use as you can think of how the gap compares to one cell of the grid, rather than how it compares to the whole plot area... do you agree or should I switch to what we did in python?

I prefer your implementation 👌

🔪 gap and made the defaults (for independent axes, not for coupled) match the python version 0.2 and 0.3 instead of the 0.25 for both I had previously. -> 670bdd5

etpinard

Thanks for taking this on! I wasn't expecting this much functionality in your first push 💪

I made a few comments. The gap vs xgap/ygap is probably the most blocking one.

etpinard · 2018-02-23T19:52:35Z

test/image/mocks/grid_subplot_types.json

+      "type": "table",
+      "header": {"values": ["a", "b"]},
+      "cells": {"values": [["c", "d"], ["e", "f"]]},
+      "domain": {"row": 2, "column": 0}


Hmm. Something is off in the baseline image above:

Does it look ok outside of nw.js?

huh, seems like a table rendering issue. Here's what I see in test_dashboard:

And if I move it a little bit, I can get the other side to (almost) disappear:

Lets treat this as a separate issue?

Lets treat this as a separate issue?

yep that's fine -> https://github.com/plotly/plotly.js/issues/new

etpinard · 2018-02-23T19:56:03Z

src/lib/coerce.js

-        otherOpts: ['dflt', 'freeLength'],
+        // set dimensions=2 for a 2D array
+        // `items` may be a single object instead of an array, in which case
+        // `freeLength` must be true.


Good stuff!

Would you mind updating the updatemenus and sliders attributes

so that they use items: {} instead of the not-as-general items: [{}, {}, {}].

etpinard · 2018-02-23T19:56:28Z

src/lib/coerce.js

+
+                exports.valObjectMeta[opts.valType].coerceFunction(v, propPart, dflt, opts);
+
+                return out;


Amazing thanks!

etpinard · 2018-02-23T19:59:57Z

test/jasmine/tests/lib_test.js

+                    .toEqual([[1, 5], [6, 10]]);
+            });
+
+            it('supports unbounded 2D freeLength arrays', function() {


Thanks for adding Lib.validate tests!

Would it too much to ask to add a Plotly.validate test for the new grid attribute?

Good call, turns out Plotly.validate didn't dig into info_array - was there any reason for that? Anyway, now it does -> 40dd784
I had to tweak the behavior there, '' vs undefined - seems reasonable, shouldn't affect behavior but cleans up validate.

etpinard · 2018-02-23T20:13:28Z

src/plots/grid.js

+            'then iterating rows according to `roworder`.'
+        ].join(' ')
+    },
+    gap: {


I'd vote to 🔪 gap and stick to xgap and ygap. The python api make_subplots only uses horizontal_spacing and vertical_spacing with different default values:

Moreover, I prefer gap over gutter and grout (which is a word I never heard before 📚 ), but this isn't a strong opinion.

etpinard · 2018-02-23T20:49:52Z

src/plots/grid.js

+            'that each x axis is used in. *top* and *top plot* are similar.'
+        ].join(' ')
+    },
+    yside: {


Nice touch here 👌

etpinard · 2018-02-23T20:53:07Z

src/plots/domain.js

@@ -69,4 +71,60 @@ module.exports = function(opts, extra) {
        }),
        editType: opts.editType
    };
+
+    if(!opts.noGridCell) {


I don't see any noGridCell options being set. 🔪 ?

noGridCell shows up in the domain for the grid itself - you can't put the grid in itself! 😄

etpinard · 2018-02-23T20:55:57Z

src/plots/domain.js

+    if(!opts.noGridCell) {
+        out.row = {
+            valType: 'integer',
+            min: 0,


Two suggestions:

It might be worth adding a dflt: 0, so that domain: {column: 2} defaults to subplot xy2?

It might be nice to allow domain: {row: ''} to match the 'x'/'x2'/'x3' counter

It might be worth adding a dflt: 0, so that domain: {column: 2} defaults to subplot xy2?

Yeah, makes sense - if you have a grid, by default all subplots should go in it. You can always set explicit domain.x and domain.y anyhow, this is only setting the defaults for those attributes -> 4b43e35

It might be nice to allow domain: {row: ''} to match the 'x'/'x2'/'x3' counter

As discussed offline, I omitted this one (though if you say '' you'll get 0 now anyway with the change ^^ so the result is the same)

etpinard · 2018-02-23T20:59:06Z

test/image/mocks/grouped_bar.json

@@ -33,11 +33,6 @@
    "xaxis": {
      "type": "category"
    },
-    "barmode": "group",
-    "categories": [


This mock is ancient. Way older than the axis.categories attribute. This must be a typo. 👍

etpinard · 2018-02-23T21:06:59Z

src/plots/grid.js

+            '*xy* or *x3y2*, or ** to leave that cell empty. You may reuse x axes',
+            'within the same column, and y axes within the same row.',
+            'Non-cartesian subplots and traces that support `domain` can place themselves',
+            'in this grid separately using the `gridcell` attribute.'


... and also allowing multiple grids could be a nice way to generate grids with multi-colspan, multi-rowspan cells and crazy shared axes pattern.

Apparently I missed parcoords in the Plotly.react cleanup

still don't know why it's failing... but it seems to work when run in a smaller batch

alexcjohnson · 2018-02-26T04:28:15Z

I never did figure out what was causing the failing test, so I just marked it @flaky and it runs there. Not exactly the intent of @flaky, as this test simply fails in the @gl suite now on CI (but not locally, for me anyway). I found an issue with the test suite that I hoped would show what was going on - dropped promise errors - but no luck there (did lots of testing on the grid-ci-test branch), and I noticed that I forgot to update parcoords for Plotly.react... but never could figure out why that test stalls on CircleCI. I hope you don't mind @etpinard 😓

etpinard · 2018-02-26T15:40:30Z

src/traces/parcoords/calc.js

    }

    return wrap({
        lineColor: color,
        cscale: cscale
    });
 };
+
+function constHalf(len) {


Thanks for making this more readable albeit less functional. 👌

etpinard · 2018-02-26T15:45:19Z

src/traces/parcoords/defaults.js

+            colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'line.', cLetter: 'c'});
+            // TODO: I think it would be better to keep showing lines beyond the last line color
+            // but I'm not sure what color to give these lines - probably black or white
+            // depending on the background color?


I agree here.

Currently, scatter traces with marker.color arrays shorter than the coordinates get black markers - regardless of plot_bgcolor. Adding some logic around the background color would be even better.

This probably deserves a new issue.

Great -> #2405

etpinard · 2018-02-26T15:58:43Z

test/jasmine/tests/validate_test.js

+        assertErrorContent(
+            out[4], 'dynamic', 'layout', null,
+            ['xaxis', 'range', 1], 'xaxis.range[1]',
+            'In layout, key xaxis.range[1] (set to \'lots\') got reset to \'50\' during defaults.'


Hmm. Why does xaxis.range[1] get reset to '50'? here? I thought the xaxis.range[1] default was 6?

It gets range[0] * 10 https://github.com/plotly/plotly.js/blob/master/src/plots/cartesian/set_convert.js#L324

ok thanks 👌

etpinard · 2018-02-26T16:01:24Z

src/plots/domain.js

@@ -90,6 +91,7 @@ exports.attributes = function(opts, extra) {
        out.column = {
            valType: 'integer',
            min: 0,
+            dflt: 0,


Great. Can we 🔒 this down in a jasmine test?

good call -> 794669b

etpinard · 2018-02-26T16:05:16Z

so I just marked it @flaky and it runs there. Not exactly the intent of @flaky, as this test simply fails in the @gl suite now on CI

That's how many of @flaky tests behave unfortunately. When someone discovers flaky behavior locally, we should really make an effort to fix it. In brief, the @flaky tag is reserved for tests that don't behave consistently on CI - which are much harder to fix.

I hope you don't mind @etpinard sweat

I'm happy with parcoords tests that ensure we don't slice arrays during the defaults.

etpinard · 2018-02-26T20:31:24Z

Great. Looks like all my comments have been addressed. 💃

alexcjohnson added 4 commits February 23, 2018 11:28

rm ignored attr in grouped_bar mock

001bd9b

get tests running again on AJ's machine

606a601

generalize info_array to 2D and arb. length

f01db07

layout.grid

82fbae2

alexcjohnson commented Feb 23, 2018

View reviewed changes

alexcjohnson added feature something new status: reviewable labels Feb 23, 2018

etpinard added this to the v1.35.0 milestone Feb 23, 2018

etpinard reviewed Feb 23, 2018

View reviewed changes

alexcjohnson mentioned this pull request Feb 23, 2018

Table borders sometimes get cut off #2401

Closed

alexcjohnson added 7 commits February 23, 2018 18:20

catch promise errors in parcoords_test

cd6e823

clean up parcoords handling of uneven and missing arrays

1aec7f7

Apparently I missed parcoords in the Plotly.react cleanup

try marking the test that's failing on CI only as flaky

446f9f9

still don't know why it's failing... but it seems to work when run in a smaller batch

validate info_array, make grid defaults work better with validate

40dd784

🔪 grid.gap

670bdd5

drop domain and grid editTypes from calc to plot

6b8d461

row and column default to 0 in domain attrs

4b43e35

etpinard reviewed Feb 26, 2018

View reviewed changes

alexcjohnson mentioned this pull request Feb 26, 2018

uneven arrays in parcoords #2405

Closed

default subplot row/column test

794669b

etpinard mentioned this pull request Feb 26, 2018

Better schema declarations for array attributes that don't scale with coordinate arrays #1894

Closed

alexcjohnson merged commit 77c218a into master Feb 26, 2018

alexcjohnson deleted the layout-grid branch February 26, 2018 23:56

This was referenced Feb 27, 2018

Plotly.purge in parcoords_test #2406

Merged

multiple selections on parcoords axes #2415

Merged

etpinard mentioned this pull request Mar 1, 2018

ZERO circular dependency #2429

Merged

alexcjohnson mentioned this pull request Aug 1, 2018

Parallel Categories trace type for multi dimensional categorical data jonmmease/plotly.js#1

Closed

4 tasks

lumip mentioned this pull request Jan 23, 2024

Improved hover detection for scatter plot fill tonext* #6865

Merged


		exports.valObjectMeta[opts.valType].coerceFunction(v, propPart, dflt, opts);

		return out;

Layout grids #2399

Layout grids #2399

Conversation

alexcjohnson commented Feb 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

alexcjohnson commented Feb 26, 2018

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 Feb 26, 2018 • edited Loading

etpinard commented Feb 26, 2018

etpinard commented Feb 26, 2018 •

edited

Loading