Handle 'missing' matching axes #4529

etpinard · 2020-01-29T22:24:20Z

fixes #4501 - attempts to supersede @archmoj 's #4506 by implementing "solution 1" as presented in #4501 (comment)

Examples:

minimal case where we have traces on subplots x2y2, x3y3, x4y4, all x-axes have matches: 'x', all y-axes have matches: 'y'
- off v1.52.1: https://codepen.io/MojtabaSamimi/pen/wvBRGyV - matches: 'x' and matches: 'y' are invalid, and the axes are not linked
- off this branch: https://codepen.io/etpinard/pen/gObJBXm?editors=1010 matches: 'x' and matches: 'y' are now valid
same as previous, but with a set xaxis.range
- off @archmoj 's Match Cartesian axes with identical invisible reference #4506 branch: https://codepen.io/alexcjohnson/pen/wvBNXgY?editors=0010 - that xaxis.range setting gets ignored
- off this branch: https://codepen.io/etpinard/pen/KKwLGZe?editors=1010 - the xaxis.range gets coerced and is the value used for the matching x-axes
similar to previous, but now with traces on subplots x3y3 and x4y4 only and x4 matching x3, x3 matching x2 and x2 matching x
- this case is also working off this branch: https://codepen.io/etpinard/pen/GRgaYxW

Pinging @alexcjohnson @nicolaskruchten who may want to try these examples.

For the reviewers, commit 33078b0 adds a way to track (in fullLayout._axisMatchGroups) and coerce the 'missing' axes which are referenced via the matches attributes but that do not have traces on them (i.e. are not coerced into fullLayout otherwise). Commit df1c59f makes the downstream logic work: as far as I could tell we only needed to skip missing axes during Axes.drawOne and during the _axisMatchGroups autorange sync operation.

- keep list of valid but missing `matches` value - coerce the missing axes in fullLayout - include the missing axes in the list of valid `matches` values - add a few comments about the variables used in the cartesian supplyLayoutDefaults routine. - add jasmine supplyDefaults tests!

- do not consider 'missing' axes during autorange sync-up operation - do not attempt to update missing axes on drag and other relayout calls that reference missing axes

nicolaskruchten · 2020-01-29T22:41:01Z

Just to confirm: this does or could work with other attributes than range like categoryarray and alignmentgroup and so on?

alexcjohnson · 2020-01-30T00:06:33Z

Looks great! My only code comment is it may be possible to 🌴 the missing & real axis coercion code a bit, but OTOH that could make it less readable... so definitely non-⛔️

Just to confirm: this does or could work with other attributes than range like categoryarray and alignmentgroup and so on?

It certainly looks like that should work, it does a fairly complete coercion of attributes on these missing axes, making no explicit mention of range in the process. But yeah, may be nice to include tests of these other attributes.

etpinard · 2020-01-30T14:41:58Z

this does or could work with other attributes than range like categoryarray and alignmentgroup and so on?

Does anyone have a px example ready that uses categoryarray and alignmentgroup?

etpinard · 2020-01-30T14:44:14Z

My only code comment is it may be possible to the missing & real axis coercion code a bit, but OTOH that could make it less readable... so definitely non-

Yeah, I paid special attention to making this new code as readable as it can be. The cartesian supplyLayoutDefaults logic is already pretty complex.

... we'll see what @archmoj thinks about it.

src/plots/cartesian/layout_defaults.js

nicolaskruchten · 2020-01-30T18:18:29Z

Here's a PX-output case with alignmentgroup set: https://codepen.io/nicolaskruchten/pen/ExaOMRr?editable=true

nicolaskruchten · 2020-01-30T18:20:39Z

If you set categoryarray on the x axis there, we would like it to propagate.

etpinard · 2020-01-30T19:45:39Z

Thanks for the example @nicolaskruchten

Things appear to work ok with categoryarray set in the missing x axis: https://codepen.io/etpinard/pen/QWwXKPd

but one must set xaxis.type: 'category' first, because as the x axis doesn't have any traces associated with it, we get the default xaxis.type: 'linear'. Recall that matching axes of different type values is forbidden.

I suspect that setting type: 'category' isn't ideal in the PX context, so let me try to find a way to propagate the axis type through to the 'missing' axes.

etpinard · 2020-01-30T19:53:33Z

Things appear to work ok with categoryarray set in the missing x axis:

Actually, the x categories are reversed, we should show Male then Female in this case with xaxis.categoryarray: ['Male', 'Female'].

On it.

nicolaskruchten · 2020-01-30T19:54:13Z

Hmm well today PX sets categoryorder/categoryarray a little oddly and I can't remember why: it sets it on every x axis in the bottom row and every y axis in the left column. That seems like a leftover from the pre-matches days. PX could just set it on all axes though if that makes things easier. Arguably setting it on the bottom-most row is enough, as there will always be at least one valid x axis in the bottom-most row for PX plots.

nicolaskruchten · 2020-01-30T19:54:41Z

(and ditto one valid y-axis in the left-most column)

but that's a very PX-specific property.

- this does not fix any bugs as the downstream fixedrange check use !fixedrange, but it's probably a good idea to get this into fullLayout to avoid potential bugs down the road.

- now they correctly omit subplot 'xy' and axes 'x' and 'y' from the initial fullLayout

... so that their references are valid during handleConstraintDefaults

... so that if ever a `categoryarray` is set in a missing axis, it gets correctly linked with the other axes in the match group. ++ add image mock!

etpinard · 2020-01-30T23:30:54Z

Axis type propagation and categoryarray behaviour on missing axes is now fixed:

https://codepen.io/etpinard/pen/MWYMpGm

@archmoj this PR is ready for another round of review.

test/jasmine/tests/cartesian_interact_test.js

test/image/compare_pixels_test.js

archmoj · 2020-01-31T15:54:19Z

@etpinard thanks very much for this great PR.

💃 💃 💃
💃 💃 💃
💃 💃

archmoj and others added 4 commits January 29, 2020 14:35

move gl2d comment

3666cf0

some linting in plots/cartesian/

0984974

make interactions for matched group w/ missing axes work

df1c59f

- do not consider 'missing' axes during autorange sync-up operation - do not attempt to update missing axes on drag and other relayout calls that reference missing axes

etpinard added bug something broken status: reviewable labels Jan 29, 2020

archmoj mentioned this pull request Jan 30, 2020

Match Cartesian axes with identical invisible reference #4506

Closed

archmoj reviewed Jan 30, 2020

View reviewed changes

src/plots/cartesian/layout_defaults.js Show resolved Hide resolved

etpinard added 6 commits January 30, 2020 16:53

coerce fixedrange in 'missing' axis loop

330bde1

- this does not fix any bugs as the downstream fixedrange check use !fixedrange, but it's probably a good idea to get this into fullLayout to avoid potential bugs down the road.

fixup new missing matching axis jasmine tests

22b34c0

- now they correctly omit subplot 'xy' and axes 'x' and 'y' from the initial fullLayout

copy axis type into missing axes

488c35f

... so that their references are valid during handleConstraintDefaults

link up missing axes during setupAxisCategories

0230b73

... so that if ever a `categoryarray` is set in a missing axis, it gets correctly linked with the other axes in the match group. ++ add image mock!

fixup (add fallback for _axisMatchGroups)

6b61e34

add treemap_with-without_values to list of flaky mocks

f151aed

archmoj reviewed Jan 31, 2020

View reviewed changes

test/jasmine/tests/cartesian_interact_test.js Show resolved Hide resolved

archmoj reviewed Jan 31, 2020

View reviewed changes

test/image/compare_pixels_test.js Show resolved Hide resolved

modify the list of flaky mocks

2589451

etpinard merged commit 6cf8b64 into master Jan 31, 2020

etpinard deleted the axis-matches-with-trace-less-axis branch January 31, 2020 16:04

snyk-bot mentioned this pull request Mar 18, 2020

[Snyk] Upgrade plotly.js from 1.52.0 to 1.52.2 beauremus/ftp-viewer#19

Open

This was referenced Apr 17, 2020

[Snyk] Upgrade plotly.js from 1.41.3 to 1.52.3 dunzoit/redash#4

Open

[Snyk] Upgrade plotly.js from 1.52.1 to 1.53.0 mc-ireiser/vue-plotly#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle 'missing' matching axes #4529

Handle 'missing' matching axes #4529

etpinard commented Jan 29, 2020

nicolaskruchten commented Jan 29, 2020

alexcjohnson commented Jan 30, 2020

etpinard commented Jan 30, 2020

etpinard commented Jan 30, 2020 •

edited

Loading

nicolaskruchten commented Jan 30, 2020

nicolaskruchten commented Jan 30, 2020

etpinard commented Jan 30, 2020

etpinard commented Jan 30, 2020 •

edited

Loading

nicolaskruchten commented Jan 30, 2020

nicolaskruchten commented Jan 30, 2020

etpinard commented Jan 30, 2020

archmoj commented Jan 31, 2020

Handle 'missing' matching axes #4529

Handle 'missing' matching axes #4529

Conversation

etpinard commented Jan 29, 2020

nicolaskruchten commented Jan 29, 2020

alexcjohnson commented Jan 30, 2020

etpinard commented Jan 30, 2020

etpinard commented Jan 30, 2020 • edited Loading

nicolaskruchten commented Jan 30, 2020

nicolaskruchten commented Jan 30, 2020

etpinard commented Jan 30, 2020

etpinard commented Jan 30, 2020 • edited Loading

nicolaskruchten commented Jan 30, 2020

nicolaskruchten commented Jan 30, 2020

etpinard commented Jan 30, 2020

archmoj commented Jan 31, 2020

etpinard commented Jan 30, 2020 •

edited

Loading

etpinard commented Jan 30, 2020 •

edited

Loading