Axis constraints #1522

alexcjohnson · 2017-03-28T05:57:21Z

Closes #272 - defines and enforces constraints between axis scales, so you can have a fixed aspect ratio plot. Allows arbitrary chaining of many axes together, each with its own aspect ratio, along with unconstrained subplots.

TODO:

decide what to do if one of the axes with a constraint is also labeled fixedrange - should we forbid this combination? Or make the whole constraint group fixed? Or something else? I can't think of a use case for this so I'm inclined to forbid it, at least for now.
tests for the dynamic behavior
gl2d compatibility? This might be as simple as constraining the zoom box, given the limited zoom functionality available in gl2d right now, but there is some weird behavior right now that may also need to be addressed (open http://localhost:3000/devtools/test_dashboard/#gl2d_stacked_subplots and click "autoscale", for example)
enforce constraints in relayout calls that don't come from the GUI. This may actually allow me to simplify some things in dragbox.js too...
allow x-x and y-y constraints, not just x-y

@etpinard @cpsievert @rreusser I'd be grateful if one or two of you could play with the interactions - zoom box, dragging an axis end, dragging a corner of a plot - and tell me if they feel right. The mock in the PR - http://localhost:3000/devtools/test_dashboard/#axes_scalewith if you run the test dashboard - should be a good starting point, as it has two linkages across two coupled subplots.

all constraints are satisfied on initial plot, but not yet on zooms

etpinard · 2017-03-28T14:11:06Z

src/plots/cartesian/dragbox.js

@@ -578,7 +580,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {

        redrawObjs(fullLayout.annotations || [], Registry.getComponentMethod('annotations', 'drawOne'));
        redrawObjs(fullLayout.shapes || [], Registry.getComponentMethod('shapes', 'drawOne'));
-        redrawObjs(fullLayout.images || [], Registry.getComponentMethod('images', 'draw'));
+        redrawObjs(fullLayout.images || [], Registry.getComponentMethod('images', 'draw'), true);


Nice touch 🐎

etpinard · 2017-03-28T14:19:31Z

src/plots/cartesian/dragbox.js


                // This is specifically directed at scatter traces, applying an inverse
                // scale to individual points to counteract the scale of the trace
                // as a whole:
                .select('.scatterlayer').selectAll('.points').selectAll('.point')
-                    .call(Drawing.setPointGroupScale, 1 / xScaleFactor, 1 / yScaleFactor);
+                    .call(Drawing.setPointGroupScale, xScaleFactor, yScaleFactor);


Is this a bug currently?

no, I just flipped the definitions of x/yScaleFactor since I noticed they were being inverted in most of their uses.

alexcjohnson · 2017-03-28T14:34:34Z

src/plots/cartesian/dragbox.js

+        // (or autoranged, if we have no initial range, to match the logic in
+        // doubleClickConfig === 'reset' below), we reset.
+        // If they are *all* at their initial ranges, then we autosize.
+        if(doubleClickConfig === 'reset+autosize') {


This reorg was to fix a subtle bug:

set explicit initial ranges for both x and y,

autoscale one of them (doubleclick the middle of the x axis, for example)

doubleclick the plot repeatedly: it will toggle between autoscaled x, initial y, and autoscaled y, initial x (with this change it toggles between fully autoscaled and fully initial-scaled).

Great. Let's make sure to 🔒 this down before merging.

🔒 : 86e0d5a#diff-200dbdf53958cf8973413a709518f4bbR313

alexcjohnson · 2017-03-28T14:45:20Z

src/plots/cartesian/dragbox.js

@@ -135,6 +159,8 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {
                clearSelect(zoomlayer);
            }
            else if(isSelectOrLasso(dragModeNow)) {
+                dragOptions.xaxes = xa;
+                dragOptions.yaxes = ya;


Fixing what I broke in 98900a2#diff-1bd3dd52cddb8638876bbe17cef28a0dL105 when I consolidated to use recomputeAxisLists for the initial settings too (when dragOptions isn't yet available, and I didn't see anyone using the xaxes/yaxes properties). But it turns out it was subtly broken before anyway, as the version in recomputeAxisLists used the wrong names (dragoptions.xa instead of dragoptions.xaxes).

dragbox.js is still a bit of a mess, but... I think it's a little better now.

cpsievert · 2017-03-28T16:42:06Z

@alexcjohnson the zooming on your example looks great, and even does a sensible thing in extreme cases 🍻

I am, however, thrown out a bit by the choice of expanding the relevant axis range to match scaleratio. I'm more familiar with approaches that shrink/restrict axisID.domain's domain accordingly.

alexcjohnson · 2017-03-28T18:05:18Z

@cpsievert

I am, however, thrown out a bit by the choice of expanding the relevant axis range to match scaleratio. I'm more familiar with approaches that shrink/restrict axisID.domain's domain accordingly.

Hmm, yeah, I see that in your ggplot example #272 (comment) - it's obviously not the way I think about it, but I'm not sure if I've seen precedent for this way or not - other than I guess the way we handle maps. The only way I can really see shrinking the actual axis size making sense within our structure is some sort of algorithm to automatically reduce the total plot width to minimize white space, but that seems a bit tricky to get right. I'd also say this approach strikes me as rooted in a static presentation of the plot - those long narrow subplots are not so useful once you start zooming and panning. But of course we want to be able to replicate all the plots folks feed in!

Do you think that for ggplotly's purposes you could pull out the overall plot dimensions from ggplot's results and just pass them along to plotly.js? In which case I guess it would basically be ggplot enforcing the initial constraints (with plotly.js just verifying and possibly correcting for small differences in layout) and plotly.js's role in this would primarily be in dynamic behavior.

And re: the part that disappeared from your comment above - do you think the with/width confusion merits changing the name scalewith to something else?

cpsievert · 2017-03-28T19:45:46Z

Do you think that for ggplotly's purposes you could pull out the overall plot dimensions from ggplot's results and just pass them along to plotly.js?

That's essentially what I'm doing in plotly/plotly.R#509. As you mention, it unfortunately requires estimating the axis dimensions, which is approximate, at best. It also significantly complicates the logic on my end, but I suppose it's possible.

And re: the part that disappeared from your comment above - do you think the with/width confusion merits changing the name scalewith to something else?

I'm not highly opposed to the name, but I see your point. How about scaleanchor?

FWIW, at least for the R package, I'm going to add more robust name checking that should catch instances like this.

etpinard · 2017-03-28T20:27:26Z

[from @cpsievert] How about scaleanchor?

👍 much better than scalewith in my mind.

etpinard · 2017-03-28T20:39:58Z

decide what to do if one of the axes with a constraint is also labeled fixedrange - should we forbid this combination? Or make the whole constraint group fixed? Or something else? I can't think of a use case for this so I'm inclined to forbid it, at least for now.

I'd vote for skipping coerce('scale*') when fixedrange is turned on. The scale* and fixedrange attributes sound to me like two conflicting things.

gl2d compatibility? This might be as simple as constraining the zoom box, given the limited zoom functionality available in gl2d right now, but there is some weird behavior right now that may also need to be addressed (open http://localhost:3000/devtools/test_dashboard/#gl2d_stacked_subplots and click "autoscale", for example)

That would require a few patches in gl-select-box which could use some ❤️ at the moment. Maybe it's worth investigating using cartesian/dragbox.js for gl2d zoom-box interactions instead? I don't see a performance benefit from using a gl solution for zoom-box. Though, hovering, panning and scrolling interactions would have to remain in the gl universe for speed 🐎

alexcjohnson · 2017-03-28T20:46:52Z

I'd vote for skipping coerce('scale*') when fixedrange is turned on. The scale* and fixedrange sound to me like two conflicting things.

👍

That would require a few patches in gl-select-box which could use some ❤️ at the moment. Maybe it's worth investigating using cartesian/dragbox.js for gl2d zoom-box interactions instead?

I'll take a look at it, that might not be too hard. I ran into a few more issues gl2d zoom on multi-subplot plots - the one I mentioned above seems to be dropped contexts, the relayout makes new ones for some reason; but there's another one that the grey overlay covers the whole plot (margins included) EXCEPT whatever other gl subplots happen to be in front of the plot you're zooming on. That probably wouldn't be hard to fix but if we just swap in dragbox.js it would be moot.

cpsievert · 2017-03-29T16:25:52Z

Do you think that for ggplotly's purposes you could pull out the overall plot dimensions from ggplot's results and just pass them along to plotly.js?

After thinking about this again, I wouldn't be able to support aspect ratios in ggplotly() on resize (in such a way that mimics ggplot2's spacing). That's because axis domains (computed in R) depend on the container height/width (which may change after we lose connection to an R session). So, if it isn't a lot of trouble, it would be a win for ggplotly() if we could implement the other way (shrinking the domain of the appropriate axis). That being said, I see your point that ggplot2's approach is "rooted in a static presentation of the plot" in that current implementation is more convenient for zooming.

I guess what I'm trying to say is I would still slightly prefer axis shrinking, but I can make do with the current implementation (and understand why you prefer it). We'll just have to educate ggplotly() users why the spacing is different. I've tried to stick to the philosophy of "do our damnest to replicate the static result by default", but I suppose we can deviate from that somewhat ;)

alexcjohnson · 2017-03-29T17:25:34Z

After a discussion with @cpsievert I think we'll leave the general structure as it is and let ggplotly handle explicitly setting the subplot shapes to match what ggplot generates.

But in the discussion it came up that there are cases you want to constrain several x (or y) axis scales without a corresponding x<->y constraint. See eg plotly/plotly.R#908 - so I'll see if I can add that in here too, letting scaleanchor reference ANY other axis, regardless of its orientation. I think there's a good chance that it'll "just work" ™️ but we'll see.

plus a couple small bugfixes encountered in the process

alexcjohnson · 2017-03-31T03:22:31Z

test/jasmine/tests/click_test.js

@@ -771,7 +771,7 @@ describe('Test click interactions:', function() {
            var translate = Drawing.getTranslate(mockEl),
                scale = Drawing.getScale(mockEl);

-            expect([translate.x, translate.y]).toBeCloseToArray([61.070, 97.712]);
+            expect([translate.x, translate.y]).toBeCloseToArray([-25.941, 43.911]);


This test was actually incorrect, as wheel events weren't passing along the mouse coordinates before 86e0d5a#diff-d48932d4d7f81cc4201054d397d53f1aR17 and these matter to wheel zoom.
(the change from -1000 to -20 above is irrelevant, this event handler clips at +/-20 per event fired)

Thanks 👍

alexcjohnson · 2017-04-01T00:34:18Z

src/plot_api/plot_api.js

+                    ai.indexOf('calendar') !== -1 ||
+                    ai.match(/^(bar|box|font)/)) {
+                flags.docalc = true;
+            }


had to move this block above the following _has('gl2d') block, so scaleanchor/scaleratio get the necessary recalc. Seems like as a general rule, we might want to reorder these from biggest change to smallest, ie all the recalcs, then all the replots, then the ticks & styles... if we don't find an altogether better way to manage these flags (via the schema?).

I really like the idea of adding a recalc true flag to the attributes that require a recalc on restyle / relayout. But that can wait. We should do this in one PR crossing off #648.

alexcjohnson · 2017-04-01T00:37:54Z

src/plots/gl2d/camera.js

+
+                            // gl-select-box clips to the plot area bounds,
+                            // which breaks the axis constraint, so don't allow
+                            // this box to go out of bounds


in contrast, svg lets you make these constrained boxes extend outside the plot area. Didn't seem worth diving into gl-select-box to make this marginal feature match up precisely.

Agreed. No need to do this for now 👍

alexcjohnson · 2017-04-01T00:50:27Z

src/plots/gl2d/camera.js

+                    // otherwise clamp small changes to the origin so we get 1D zoom
+                    else {
+                        if(smallDx) result.boxEnd[0] = result.boxStart[0];
+                        if(smallDy) result.boxEnd[1] = result.boxStart[1];


@etpinard while I was at it I added in a few of the bits of behavior used in svg, such as 1D zoom, minimum zoom size, and clamping panning to pure x (y) if dy (dx) is small enough. Do you want to take it for a spin and see what you think?

Loving it. Thanks!

alexcjohnson · 2017-04-01T00:54:40Z

src/plots/gl2d/scene2d.js

+        xaxis: this.xaxis,
+        yaxis: this.yaxis
+    };
+    enforceAxisConstraints({_fullLayout: mockLayout});


kind of hacky, but enforceAxisConstraints has to happen after the Axes.doAutoRange just above, and these aren't the gd._fullLayout axes...

A bunch of things here would break if/when we support overlaid axes...

alexcjohnson · 2017-04-01T00:56:15Z

src/plots/gl2d/scene2d.js

+    }
+
+    // Remove hover effects if we're not over a point OR
+    // if we're zooming or panning (in which case result is not set)


also to match svg

Very nice. Thanks 👍

alexcjohnson · 2017-04-01T03:36:41Z

test/jasmine/tests/gl2d_click_test.js

@@ -246,12 +246,20 @@ describe('Test hover and click interactions', function() {
            pointNumber: 0
        });

+        // after the restyle, autorange changes the y range


For some reason I don't understand, when this test ran before my changes, the y-axis didn't get autoranged after the restyle, even though when I make the same restyle call on master in the test dashboard it does autorange. Anyway, now it does get autoranged during the test, as it should, which is why I needed to alter the run the second time around.

alexcjohnson · 2017-04-01T03:38:07Z

test/jasmine/tests/gl2d_click_test.js

+        // after the restyle, autorange changes the x AND y ranges
+        // I don't get why the x range changes, nor why the y changes in
+        // a different way than in the previous test, but they do look
+        // correct on the screen during the test.


I think it makes sense that the axis ranges are different here than in the test above, as this is scattergl-fancy... but then why are they the same as the test above during the first run? This test suite baffles me...

Yeah. Maybe I tried to be too fancy in this test suite.

alexcjohnson · 2017-04-01T04:15:09Z

@etpinard ready for another review. Single gl2d plots work well with scale constraints now (please try it out and let me know what you think). I did not investigate multiple gl2d subplots (coupled or otherwise) as I was thrown off for a while by #1200 (comment) but now that I understand that I could look into this, if we think it's important.

etpinard · 2017-04-03T14:31:06Z

test/jasmine/assets/mouse_event.js

@@ -14,7 +14,7 @@ module.exports = function(type, x, y, opts) {
        ev;

    if(type === 'scroll') {
-        ev = new window.WheelEvent('wheel', opts);
+        ev = new window.WheelEvent('wheel', Object.assign({}, fullOpts, opts));


can you use Lib.extendFlat here instead - in case we run these tests on old browsers some day?

done in 745b953

I should really be reviewing all commits before making comments.

etpinard · 2017-04-03T14:35:56Z

test/jasmine/tests/axes_test.js

+
+        afterEach(destroyGraphDiv);
+
+        it('updates ranges when adding, removing, or changing a constraint', function(done) {


Nice test 🎉

etpinard · 2017-04-03T16:07:59Z

💃 if you don't feel like including a patch for #1200 in this PR.

This gets my vote for PR of the year 🏆

alexcjohnson · 2017-04-03T16:52:21Z

Thanks @etpinard 😊 I'll put a note about this into #1200 but I think I should move on for now.

martintburgess · 2019-01-21T02:04:02Z

@alexcjohnson, I am trying to build a plot that is:

responsive to changes in window size (using config={'responsive': true})
NOT zoomable (both axes have fixedrange: true
has a fixed aspect ratio/uniform axis units.
Am I correct in thinking this is not possible because scaleanchor does not work with fixedrange with this PR? Thanks for your help.

alexcjohnson added 5 commits March 28, 2017 00:59

split out axis type defaults and calculate before anything else

e9a89c2

static axis scalewith/scaleratio constraints

d9a8ab7

all constraints are satisfied on initial plot, but not yet on zooms

short-sircuit image redraw during drags

6c60a32

refactor dragbox

98900a2

dynamic axis constraints

84d95a5

alexcjohnson added status: in progress feature something new labels Mar 28, 2017

etpinard added this to the v1.26.0 milestone Mar 28, 2017

etpinard reviewed Mar 28, 2017

View reviewed changes

alexcjohnson commented Mar 28, 2017

View reviewed changes

alexcjohnson added 2 commits March 29, 2017 00:01

push final constraint management into relayout so it always works

f521227

scalewith -> scaleanchor

13f848e

alexcjohnson added 4 commits March 29, 2017 15:22

forbid scale constraints along with fixedrange

dee136d

allow constraints with same axis letter

ef3ca48

little test fix

25c2cfb

test dynamic axis ranging with and without constraints

86e0d5a

plus a couple small bugfixes encountered in the process

alexcjohnson commented Mar 31, 2017

View reviewed changes

alexcjohnson added 3 commits March 30, 2017 23:32

don't use Object.assign even in tests, just in case

745b953

squish interaction test plot vertically so it fits on screen

7fc9cbd

make sure scaleanchor/scaleratio changes trigger a recalc

ffd65fd

make axis constraints work with gl2d

b4d11e6

alexcjohnson commented Apr 1, 2017

View reviewed changes

alexcjohnson added status: reviewable and removed status: in progress labels Apr 1, 2017

fix gl2d_click_test

23859e3

alexcjohnson commented Apr 1, 2017

View reviewed changes

etpinard reviewed Apr 3, 2017

View reviewed changes

alexcjohnson mentioned this pull request Apr 3, 2017

Smarter defaults for axis anchors #1200

Closed

alexcjohnson merged commit b19a87f into master Apr 3, 2017

alexcjohnson deleted the axis-constraints branch April 3, 2017 16:52

alexcjohnson mentioned this pull request Apr 3, 2017

Matching axes #1549

Closed

jackparmer mentioned this pull request Apr 5, 2017

way to preserve square aspect ratio plotly/plotly.py#70

Closed

etpinard mentioned this pull request Apr 10, 2017

[WIP] Carpet plots #1239

Closed

44 tasks

chriddyp mentioned this pull request Apr 17, 2017

aspect ratio docs and backlinks plotly/documentation#747

Closed

6 tasks

alexcjohnson mentioned this pull request Jun 8, 2017

Constrain axes by domain #1767

Merged

martintburgess mentioned this pull request Jan 20, 2019

Aspect ratio key for 2d plots #272

Closed

etpinard mentioned this pull request Jan 22, 2019

Allow fixedrange axes to scaleanchor with constrain:domain #3460

Closed


		afterEach(destroyGraphDiv);

		it('updates ranges when adding, removing, or changing a constraint', function(done) {

Axis constraints #1522

Axis constraints #1522

Conversation

alexcjohnson commented Mar 28, 2017 • 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

alexcjohnson Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

cpsievert commented Mar 28, 2017 • edited Loading

alexcjohnson commented Mar 28, 2017

cpsievert commented Mar 28, 2017

etpinard commented Mar 28, 2017

etpinard commented Mar 28, 2017 • edited Loading

alexcjohnson commented Mar 28, 2017

cpsievert commented Mar 29, 2017

alexcjohnson commented Mar 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

alexcjohnson commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 3, 2017

alexcjohnson commented Apr 3, 2017

martintburgess commented Jan 21, 2019 • edited Loading

alexcjohnson commented Mar 28, 2017 •

edited

Loading

alexcjohnson Mar 28, 2017 •

edited

Loading

cpsievert commented Mar 28, 2017 •

edited

Loading

etpinard commented Mar 28, 2017 •

edited

Loading

martintburgess commented Jan 21, 2019 •

edited

Loading