Multi-axis-type sploms #2899

etpinard · 2018-08-13T19:29:05Z

This PR fixes axis tick rendering for categorical sploms For example, on master

Plotly.newPlot(gd, [{
  type: 'splom',
  dimensions: [{
    values: ['a','b','d']
  }, {
    values: ['a', 'b', 'c']
  }]
}])

renders as:

and add supports for sploms that generate axis of different types, see 9da5a1b for an example.

cc @alexcjohnson

- using trace._diag, this allows to support multiple axis type in splom-generated axes

... to make categorical axis ticks show up correctly

- note that axis type can be overwritten in layout

etpinard · 2018-08-13T19:30:26Z

src/plots/cartesian/type_defaults.js

@@ -89,9 +89,10 @@ function setAutoType(ax, data) {
    }
    else if(d0.type === 'splom') {
        var dimensions = d0.dimensions;
+        var diag = d0._diag;


Recall that _diag is:

plotly.js/src/traces/splom/defaults.js

Lines 104 to 124 in def6aa5

// build list of [x,y] axis corresponding to each dimensions[i],

// very useful for passing options to regl-splom

var diag = traceOut._diag = new Array(dimLength);

// cases where showDiag and showLower or showUpper are false

// no special treatment as the xaxes and yaxes items no longer match

// the dimensions items 1-to-1

var xShift = !showDiag && !showLower ? -1 : 0;

var yShift = !showDiag && !showUpper ? -1 : 0;

for(i = 0; i < dimLength; i++) {

var dim = dimensions[i];

var xa = xaxes[i + xShift];

var ya = yaxes[i + yShift];

fillAxisStash(layout, xa, dim);

fillAxisStash(layout, ya, dim);

// note that some the entries here may be undefined

diag[i] = [xa, ya];

}

ah ok - a little confusing as usually xa is a full axis object, an id would be xId or something... but anyway nice fix!

cleaned up in -> f3c73db

etpinard · 2018-08-13T19:33:12Z

test/image/mocks/splom_multi-axis-type.json

+      "r": 30,
+      "t": 30
+    },
+    "yaxis2": {"type": "category"},


This here might be tricky to find for users as e.g. nothing indicates that yaxis2 corresponds to the boolean data array.

Perhaps we could include this in the trace dimensions[i] containers?

Perhaps we could include this in the trace dimensions[i] containers?

That's a great idea - dimension.type = 'category' to override the autotype for both axes. That would certainly reduce (though not eliminate) the chance of mismatches like #2899 (comment)

I went with dimensions[i].axis.type in 39b71bb

I'm thinking the extra container might make it cleaner if we add more "splom-generated-axis" settings e.g. I suspect some users would want to set dimensions[i].axis.showgrid: false

haha exactly my rationale in #2899 (comment) - I should read all your comments before making my own. Anyway, love it.

alexcjohnson · 2018-08-13T22:18:17Z

src/traces/splom/index.js

+                makeCalcdata(ya, dim);
+                if(xa && xa.type === 'category') {
+                    xa._categories = ya._categories.slice();
+                }


How could you get here? if xa exists you'll skip the else if(ya) block entirely.

But this raises an interesting (if possibly nonsensical) point - what if xa.type !== ya.type? I guess this could happen if one is numeric or date and the other is categorical, or if one is linear and the other is log. If the answer is "all hell breaks loose", which wouldn't surprise me, it may be appropriate to just log a warning and hide that dimension entirely. That seems like an abuse of the concept of a splom.

How could you get here? if xa exists you'll skip the else if(ya) block entirely.

Good point. I wrote that up to preserve symmetry, but didn't bother checking. Thanks!

what if xa.type !== ya.type? I guess this could happen if one is numeric or date and the other is categorical, or if one is linear and the other is log.

I don't think this can happen on graphs with only 1 splom trace and no ax.type set in the layout. That's because (after this PR's 1st commit dd563bb), both x and y axes use the same data array in the axis autotype routine.

Scenarios where xa.type !== ya.type can happen if,

a trace before the splom trace in gd.data autotyped the axes

a users sets ax.type in the layout

which are things that don't happen by accident.

So, I yeah I think skipping that dimension entirely makes the most sense here. Thanks for bringing this up!

a users sets ax.type in the layout
... [doesn't] happen by accident.

Hmm could happen in the editor though. @nicolaskruchten @VeraZab not sure how best to handle this. If we add dimension.type we should steer people there somehow when there's a splom on that axis. But if they still set an axis type directly would we want to find the corresponding opposite axis in the splom and set it to match, to avoid this error?

hoo boy. Well right now we don't support SPLOMs yet, and I'm thinking that when we do we should lock down axes/subplots quite tightly so that people can't break them too much :)

Skipping dims with axes of conflicting types in b3d27bd

... which simplifies logic downstream

... with other sub-supply-defaults routines

.. to more easily set axis type on splom-generated axes

etpinard · 2018-08-14T21:23:08Z

src/traces/splom/defaults.js

@@ -61,6 +61,8 @@ function dimensionDefaults(dimIn, dimOut) {

    if(!(values && values.length)) dimOut.visible = false;
    else coerce('visible');
+
+    coerce('axis.type');


Oops. I committed the new lines in splom/attributes.js in 9039a8e by accident. My bad.

alexcjohnson · 2018-08-14T21:44:09Z

test/image/mocks/splom_multi-axis-type.json

@@ -16,11 +16,13 @@
        },
        {
          "label": "bool",
-          "values": [false, true, true, true, false, true, false, false, false, true]
+          "values": [false, true, true, true, false, true, false, false, false, true],
+          "axis": {"type": "category"}


Yeah, that's way cleaner! 🎉

Good call making an axis container, I can see us wanting to allow other axis settings (gridlines, tick spacing, tick text...) to be set at the dimension level in the future.

I started thinking, since it's (generally) going to apply this to 2 axes, perhaps it should be called axes... but then that (elsewhere in the schema) implies a list of axes rather than axis properties. So I think it's good the way you have it.

alexcjohnson · 2018-08-14T21:45:15Z

test/jasmine/tests/splom_test.js

+        expect(fullLayout.yaxis2.type).toBe('category');
+    });
+
+    it('axis type setting should be skipped when dimension is not visible', function() {


agreed, good call.

alexcjohnson · 2018-08-14T21:53:18Z

test/jasmine/tests/splom_test.js

+
+        var cd = gd.calcdata[0][0];
+
+        expect(cd.t._scene.matrixOptions.data).toBeCloseTo2DArray([[2, 1, 2]]);


This is good, but I have various questions about what happens downstream from calc with the missing dimension(s)... like, cdata and ldata arrays don't match up with the dimension array anymore, does this get handled correctly in plotting? The next commit (visibleDims) seems like it assuages my concerns a bit, but would you mind including a mismatched-type dimension (somewhere in the middle of dimensions) in a mock so we can see it all the way to the end?

Good call, done in -> 390f292

alexcjohnson · 2018-08-14T21:55:48Z

src/traces/splom/index.js

@@ -59,16 +59,17 @@ function calc(gd, trace) {
            xa = AxisIDs.getFromId(gd, trace._diag[i][0]);
            ya = AxisIDs.getFromId(gd, trace._diag[i][1]);

+            // if corresponding x & y axes don't have matching types, skip dim
+            if(xa && ya && xa.type !== ya.type) continue;


Seems likely to confuse people if they encounter this issue... do you want to include a Log message, or perhaps find a way to surface this in Plotly.validate?

As this is part of the calc step (which is outside the scope of Plotly.validate) and that we can't easily move this to the defaults step (as axis defaults are coerced after trace defaults), I chose to only log something in 1823904

Note that I chose I was first planning on using Lib.warn instead of Lib.log, but currently the image server bails whenever a mock results in a Lib.warn, so the mock added in 390f292 would fail to generate an image. I hope you're ok with just a Lib.log here.

.. in case (e.g. for splom) they need to lookup a few things in fullData and calcdata

... when restyling dragmode to non-selection modes. Previously, select -> dblclick -> pan lead to a duplication of rendered pts.

etpinard · 2018-08-15T16:47:12Z

test/jasmine/tests/splom_test.js

+        .then(function() { return Plotly.relayout(gd, 'dragmode', 'pan'); })
+        .then(function() {
+            _assert('under dragmode:pan with NO active selection', {
+                updateCnt: 1,       // to clear off 1 matrixTrace


Before this commit, this

was happening.

Nice find, thanks for including the fix here!

etpinard · 2018-08-15T16:48:09Z

test/jasmine/tests/splom_test.js

+        .then(function() { return Plotly.relayout(gd, 'dragmode', 'select'); })
+        .then(function() {
+            _assert('under dragmode:select', {
+                updateCnt: 3,     // updates positions, viewport and style in 3 calls


Recall that relayout(gd, 'dragmode', 'select') results in a plot edit here because of:

plotly.js/src/plot_api/plot_api.js

Lines 1980 to 1986 in 9d570fc

if((fullLayout._has('scatter-like') && fullLayout._has('regl')) &&

(ai === 'dragmode' &&

(vi === 'lasso' || vi === 'select') &&

!(vOld === 'lasso' || vOld === 'select'))

) {

flags.plot = true;

}

... to avoid confusing with i in dimension loop in outer-scope.

alexcjohnson · 2018-08-15T20:22:55Z

test/image/mocks/splom_mismatched-axis-types.json

+      "xref": "paper", "yref": "paper",
+      "xanchor": "right", "yanchor": "top",
+      "x": 1, "y": 1,
+      "text": "Should <b>not</b> see points in the \"bool\" dimension<br>as it has conflicting axis types",


Great mock, thanks!

alexcjohnson · 2018-08-15T20:26:44Z

Excellent, this is really going to help robustness and usability for splom traces. 💃

etpinard added 3 commits August 13, 2018 15:04

consider only splom dimensions on axis during axis autotype

dd563bb

- using trace._diag, this allows to support multiple axis type in splom-generated axes

copy back axes categories on axes that skipped makeCalcdata,

9e1869d

... to make categorical axis ticks show up correctly

add splom mutli axis type mock

9da5a1b

- note that axis type can be overwritten in layout

etpinard added bug something broken feature something new status: reviewable labels Aug 13, 2018

etpinard added this to the v1.40.0 milestone Aug 13, 2018

etpinard commented Aug 13, 2018

View reviewed changes

alexcjohnson reviewed Aug 13, 2018

View reviewed changes

etpinard added 5 commits August 14, 2018 14:25

swap (x|y)a -> (x|y)id for clarity

f3c73db

skip dims with conflicting axis types

b3d27bd

keep track of visible dimensions using visibleDims list

9039a8e

... which simplifies logic downstream

bring handleTypeDefaults api on-par

ab7d9c6

... with other sub-supply-defaults routines

add dimensions[i].axis.type

39b71bb

.. to more easily set axis type on splom-generated axes

etpinard commented Aug 14, 2018

View reviewed changes

alexcjohnson reviewed Aug 14, 2018

View reviewed changes

etpinard added 4 commits August 15, 2018 10:42

pass gd to basePlotModule.updateFx

57ddadb

.. in case (e.g. for splom) they need to lookup a few things in fullData and calcdata

clear additional 'matrixTrace' use by regl-splom for selection

15efc72

... when restyling dragmode to non-selection modes. Previously, select -> dblclick -> pan lead to a duplication of rendered pts.

log info when skipping splom dim of conflicting axis types

1823904

add mock of splom with conflicting axis types

390f292

etpinard commented Aug 15, 2018

View reviewed changes

use 'j' in value loop

adb0004

... to avoid confusing with i in dimension loop in outer-scope.

alexcjohnson reviewed Aug 15, 2018

View reviewed changes

etpinard merged commit 7bba266 into master Aug 15, 2018

etpinard deleted the categorical-sploms branch August 15, 2018 20:27

etpinard mentioned this pull request Oct 2, 2018

Splom open items #2555

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-axis-type sploms #2899

Multi-axis-type sploms #2899

etpinard commented Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 14, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 14, 2018

alexcjohnson Aug 14, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 14, 2018

alexcjohnson Aug 14, 2018

nicolaskruchten Aug 14, 2018

etpinard Aug 14, 2018

etpinard Aug 14, 2018

alexcjohnson Aug 14, 2018

alexcjohnson Aug 14, 2018

alexcjohnson Aug 14, 2018

etpinard Aug 15, 2018 •

edited

Loading

alexcjohnson Aug 14, 2018

etpinard Aug 15, 2018

etpinard Aug 15, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 15, 2018

alexcjohnson Aug 15, 2018

alexcjohnson commented Aug 15, 2018

	// build list of [x,y] axis corresponding to each dimensions[i],
	// very useful for passing options to regl-splom
	var diag = traceOut._diag = new Array(dimLength);

	// cases where showDiag and showLower or showUpper are false
	// no special treatment as the xaxes and yaxes items no longer match
	// the dimensions items 1-to-1
	var xShift = !showDiag && !showLower ? -1 : 0;
	var yShift = !showDiag && !showUpper ? -1 : 0;

	for(i = 0; i < dimLength; i++) {
	var dim = dimensions[i];
	var xa = xaxes[i + xShift];
	var ya = yaxes[i + yShift];

	fillAxisStash(layout, xa, dim);
	fillAxisStash(layout, ya, dim);

	// note that some the entries here may be undefined
	diag[i] = [xa, ya];
	}


		var cd = gd.calcdata[0][0];

		expect(cd.t._scene.matrixOptions.data).toBeCloseTo2DArray([[2, 1, 2]]);

	if((fullLayout._has('scatter-like') && fullLayout._has('regl')) &&
	(ai === 'dragmode' &&
	(vi === 'lasso' \|\| vi === 'select') &&
	!(vOld === 'lasso' \|\| vOld === 'select'))
	) {
	flags.plot = true;
	}

Multi-axis-type sploms #2899

Multi-axis-type sploms #2899

Conversation

etpinard commented Aug 13, 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

etpinard Aug 15, 2018 • 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

alexcjohnson commented Aug 15, 2018

etpinard Aug 15, 2018 •

edited

Loading