Constrain axes by domain #1767

alexcjohnson · 2017-06-08T15:10:10Z

An alternative mechanism to enforce axis scale constraints: instead of just altering range as in #1522 this PR allows you to keep exactly the specified ranges, and restricts domain instead. That way no extraneous regions of the data axes are visible.

We add two new attributes to axes to handle this:

constrain: default is constrain: 'range' (gives the behavior in Axis constraints #1522), the new behavior of this PR is invoked by constrain: 'domain'
constraintoward: which direction to allow the data (either range or domain depending on constrain) to float. default is 'center' (x) or 'middle' (y) but you can also use 'left' and 'right' for x axes, and 'top' and 'bottom' for y. For example, 'left' means that the left end of the range or domain is unchanged when enforcing the constraint, so only the right end will change. This is most useful with constrain: 'domain' to control where the whitespace shows up, for example if you want the subplot to push next to a neighboring subplot or next to a legend.
both of these only have an effect if the axis is constrained, ie it and another axis are linked by scaleanchor in one of them.
these settings apply to each axis separately - if you want to constrain by domain in either direction, whichever happens to need constraining, you need to set constrain: 'domain' in both axes.

Also included here are a bunch of tweaks to unrelated test suites, so that the whole npm run test-jasmine passes on my machine. One of these fixes uncovered a bug - fixed in #1753 which was merged into this branch so this PR will get that fix into master.

cc @etpinard

I don't understand why we need to initInteractions twice, but it seems necessary to do both before and after finalDraw

standardize on assets/delay and increase the updateCamera delay

if you mouseout of the gl2d plot while hovering on a point, this ensures that unhover will be triggered. prior to this the test "scattergl after visibility restyle" consistently failed for me locally

seems like chrome has a more efficient image encoder now

alexcjohnson · 2017-06-08T15:21:50Z

src/plots/cartesian/dragbox.js

+            var axId = activeAxIds[i];
+            doTicks(gd, axId, true);
+            var ax = getFromId(gd, axId);
+            updates[ax._name + '.range'] = ax.range.slice();


@etpinard this is significantly simpler than what I was doing before (calculating the range edits again in dragTail) and more robust since it's explicitly WYSIWYG - whenever you update tick labels you record that and use it as the basis of the update object to be sent to relayout.

Zoombox has a counterpart here

Oh, I'm noticing now that previously we sent each end of the range as a separate edit in the update object (ie 'xaxis.range[0]': 1, 'xaxis.range[1]': 5 rather than 'xaxis.range': [1, 5]) - this way is more compact but I bet it'll break some applications that listen to relayout events to respond to zooms and inspect the event data to find the range changes, so I think I should change it back to the previous form.

note that one effect of doing it this way is when you drag axis ends or make a zoombox (any of the GUI interactions) while using constrain: 'domain', the shape and constrained domains of the subplot will never change because we always make proportional updates to all linked ranges. This is NOT the case for generic relayout calls: if you relayout a single range, it can reshape the subplot, potentially changing which axis is constrained, but always keeping the domains as large as possible within the constraint.

so I think I should change it back to the previous form.

I agree. Listening to plotly_relayout and inspecting the event data for xaxis.range[0] or xaxis.range[1] must be pretty common in userland.

We should try to clean that up in v2 or maybe we could add a plotly_rerange?

-> back to independent attributes in 51fc2c6

alexcjohnson · 2017-06-08T15:23:15Z

src/plots/cartesian/dragbox.js

    }

    // updateSubplots - find all plot viewboxes that should be
    // affected by this drag, and update them. look for all plots
    // sharing an affected axis (including the one being dragged)
+    // returns all the new axis ranges as an update object


oops, this was an aborted attempt, I'm not using thisattrs so will remove it.

-> removed this attrs in 3931973

alexcjohnson · 2017-06-08T15:41:59Z

One tricky aspect of this implementation: whenever ax.domain gets changed to satisfy a constraint, the input gd.layout axis domain is also changed to match, but the original full domain is stashed in ax._inputDomain, so that if you change ranges later and the subplot shape changes, it can expand to fill the originally specified space so it doesn't have to get smaller and smaller.

I considered doing this by either 1) letting the _fullLayout ax.domain be different from the layout input value, but I thought that would cause confusion & problems, particularly in the workspace; or 2) using another (private) attribute like ax._finalDomain for all actual axis positioning, but I felt like that would be too intrusive for a fairly esoteric feature.

One pitfall with this approach though is that when a plot is saved, ax._inputDomain will be lost so later relayouts will not be able to access the full original domain. This isn't a problem for pure GUI use because that can't change the shape (see #1767 (comment)) but I suppose could cause confusion if the plot is brought back into the workspace, or into some other application that permits more granular relayouts.

etpinard · 2017-06-08T16:04:34Z

cc @cpsievert who might be interested in this feature.

etpinard · 2017-06-08T16:13:47Z

test/jasmine/tests/gl2d_click_test.js

        if(sel.node() === null) {
            expect(expected.noHoverLabel).toBe(true);
            return;
        }

        var path = sel.select('path');
-        expect(path.style('fill')).toEqual(expected.bgColor, 'bgcolor');
-        expect(path.style('stroke')).toEqual(expected.borderColor, 'bordercolor');


Too bad that toEqual can't print a failure msg. Oh jasmine.

Thanks very much for those test tweaks!

yeah, didn't it used to be able to do that? super annoying.

etpinard · 2017-06-08T16:16:35Z

test/jasmine/tests/mapbox_test.js

-    var MINIMUM_LENGTH = 1e5;
+    // decreased from 1e5 - perhaps chrome got better at encoding these
+    // because I get 99330 and the image still looks correct
+    var MINIMUM_LENGTH = 8e4;


Great. As long as the image looks ok! Some background info -> #1598

etpinard · 2017-06-08T16:36:34Z

src/plot_api/plot_api.js

@@ -383,6 +383,7 @@ Plotly.plot = function(gd, data, layout, config) {
        drawFramework,
        marginPushers,
        marginPushersAgain,
+        initInteractions,


for reference (because this diff isn't great), we now have:

just want to call out a note I put in a commit message:

I don't understand why we need to initInteractions twice, but it seems necessary to do both before and after finalDraw

I'll have a perf PR coming after this merges and will look into that in more detail.

etpinard · 2017-06-08T16:37:35Z

src/plot_api/plot_api.js

@@ -190,8 +190,7 @@ Plotly.plot = function(gd, data, layout, config) {

        return Lib.syncOrAsync([
            subroutines.layoutStyles,
-            drawAxes,
-            initInteractions
+            drawAxes


Yeah, I guess initialising interactions after pushing the margins makes sense 👍

etpinard · 2017-06-08T16:43:24Z

src/plots/cartesian/axes.js

@@ -455,6 +455,13 @@ axes.expand = function(ax, data, options) {
        i, j, v, di, dmin, dmax,
        ppadiplus, ppadiminus, includeThis, vmin, vmax;

+    // domain-constrained axes: base extrappad on the unconstrained
+    // domain so it's consistent as the domain changes
+    if(extrappad && ax._inputDomain) {


Interesting choice for having a condition based on ax._inputDomain.

Wouldn't ax.constrain === 'domain' be more robust?

ah possibly... this might get called before enforceAxisConstraints though, so I guess I should filter on both.

-> tighter condition in 1271382

FWIW I think there's a good argument that basing this padding on _inputDomain is the right thing to do (rather than having the padding be 5% of the constrained domain), but a big part of MY motivation here was that trying to do it the other way means you'd have to make a totally separate attribute in the ax._min/_max items in addition to val and pad because you'd no longer be able to express the padding either in data (val) or pixel (pad) units, then both the auto-range and the auto-domain routines get more complicated... also it makes the conditions more complicated for which points might possibly be the extremes so must be kept in ax._min/max. I didn't want to open that can of worms.

But if anyone has a strong opinion that padding based on the constrained domain is truly the correct behavior we can certainly discuss. The biggest argument in that direction that I can see is that if your data make a perfect square but it's padded on constrained axes, the subplot itself will not end up precisely square. But if you've gotten to that point, perhaps you could be bothered to explicitly set up the domains and dimensions so the axes have equal length :)

etpinard · 2017-06-08T16:46:52Z

src/plots/cartesian/constraint_defaults.js

+    Lib.coerce(containerIn, containerOut, {
+        constraintoward: {
+            valType: 'enumerated',
+            values: letter === 'x' ? ['left', 'center', 'right'] : ['bottom', 'middle', 'top'],


it might time to consider having separate attribute declarations for x axes and y axes (similarly for x,y and z error bars #621). The reference page https://plot.ly/javascript/reference/ would benefit.

I'm curious, does Plotly.validate gives the correct answer here e.g. where constraintoward: 'bottom' in an xaxis container?

heh, no, Plotly.validate doesn't catch constraintoward: 'bottom' on an x axis, but it also incorrectly complains about anchor and scaleanchor:

LOG: In layout, key xaxis.scaleanchor is set to an invalid value (y) LOG: In layout, key xaxis.anchor is set to an invalid value (y)

I found a way to fix this. Can I push a commit to this branch?

Sure, push it!

Fixing Plotly.validate for scaleanchor, anchor and overlaying was a little harder than I thought actually, so I made a separate PR -> #1769

etpinard · 2017-06-08T16:48:53Z

src/constants/alignment.js

+module.exports = {
+    // from bottom left: this is the origin of our paper-reference
+    // positioning system
+    FROM_BL: {


First steps towards #1577

Nice.

Ah, wasn't even thinking of that but good point. There are tons of other places we could make use of these (potentially adding FROM_BR and FROM_TR...) to make things clearer and more efficient. see eg ag middle: in the src directory. I guess perhaps all of these could also allow [0, 1].

etpinard · 2017-06-08T16:50:32Z

src/plots/cartesian/constraints.js


-            if(normScale !== matchScale) {
-                scaleZoom(axes[axisID], normScale / matchScale);
+                    // TODO


still TODO ?

ignore. my bad

removed in a later commit I think :)

yeah sorry - TODO removed here

so that user apps that listen to relayouts get the same form as before

- so that attribute that declare possible values as regex e.g. axis anchor and overlaying pass `Lib.validate()`

- e.g. axis 'anchor' declare both x and y values, but coerce x or y values depending on the container. - similarly for 'overlaying' and 'contraintoward'

Validate dynamic enumerated

etpinard · 2017-06-09T14:17:43Z

src/plots/cartesian/dragbox.js

@@ -733,6 +735,8 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {
            var plotDx = xa2._offset - clipDx / xScaleFactor2,
                plotDy = ya2._offset - clipDy / yScaleFactor2;

+            // console.log(subplot.id, editX2, editY2, xScaleFactor2, yScaleFactor2, clipDx, clipDy, plotDx, plotDy);


🔪 please

thanks -> fa78376

etpinard · 2017-06-09T14:23:54Z

test/jasmine/assets/custom_matchers.js

-                );
+                var passed;
+
+                if(Array.isArray(actual) && Array.isArray(expected)) {


etpinard · 2017-06-09T14:48:21Z

test/jasmine/tests/axes_test.js

+
+        it('can change per-axis constrain:domain/range and constraintoward', function(done) {
+            Plotly.plot(gd,
+                // start with a heatmap as it has no padding so calculations are easy


Thanks for all these comments in this suite. They make my life a lot easier.

etpinard · 2017-06-09T14:53:15Z

test/jasmine/tests/axes_test.js

+            .then(done);
+        });
+
+        it('can constrain date axes', function(done) {


Would it be too much to ask you to add a 'can constrain category axes' test case?

hah, I set myself up for that one didn't I. Sure. I'll include 'log' too.

category and log axis domain and range constraint tests to match the category one -> 67ea3a8

etpinard · 2017-06-09T15:41:41Z

💃 💃 💃

alexcjohnson added 15 commits June 8, 2017 10:40

axis.constrain and axis.constraintoward

d0af01b

robustify plot_test

cc705c3

simplify plot_test

b3a3bb7

robustify plot_api_test

27fc2a9

simplify hover_label_test

d937732

no really, initInteractions! fix #1044... again

1570274

I don't understand why we need to initInteractions twice, but it seems necessary to do both before and after finalDraw

robustify drawing_test

72896ec

robustify gl_plot_interact_test

ee3211a

standardize on assets/delay and increase the updateCamera delay

streamline gl2d_click_test

f12ff73

fix bug with unhover in gl2d

5f5214e

if you mouseout of the gl2d plot while hovering on a point, this ensures that unhover will be triggered. prior to this the test "scattergl after visibility restyle" consistently failed for me locally

robustify mapbox_test

9df6903

seems like chrome has a more efficient image encoder now

tests of axis domain constraints

d0b15c8

create constraints.clean to get it out of the main Plotly.plot code

d67829e

more robust way to generate relayout args during axis drag

b1c6b24

update constraints.js to handle some more editing edge cases

59a092a

alexcjohnson commented Jun 8, 2017

View reviewed changes

etpinard added type: feature status: reviewable labels Jun 8, 2017

etpinard reviewed Jun 8, 2017

View reviewed changes

put range 0,1 parts separately into GUI relayout calls

51fc2c6

so that user apps that listen to relayouts get the same form as before

alexcjohnson and others added 4 commits June 8, 2017 13:17

rm unused attrs variable

3931973

axes.expand: ensure domain constraint before adjusting extrappad

1271382

add special validateFunction for enumerated valType

6d3a48a

- so that attribute that declare possible values as regex e.g. axis anchor and overlaying pass `Lib.validate()`

add special handler in Plotly.validate for enumerated w/ dynamic values

3b0d829

- e.g. axis 'anchor' declare both x and y values, but coerce x or y values depending on the container. - similarly for 'overlaying' and 'contraintoward'

etpinard mentioned this pull request Jun 8, 2017

Validate dynamic enumerated #1769

Merged

Merge pull request #1769 from plotly/validate-dynamic-enumerated

4002fc1

Validate dynamic enumerated

etpinard added this to the 1.28.0 milestone Jun 8, 2017

alexcjohnson mentioned this pull request Jun 9, 2017

Misc perf improvements #1772

Merged

etpinard referenced this pull request Jun 9, 2017

drop the unneeded initial draw of axes

b4a2f4d

etpinard reviewed Jun 9, 2017

View reviewed changes

test/jasmine/assets/custom_matchers.js

);

var passed;

if(Array.isArray(actual) && Array.isArray(expected)) {

Copy link

Contributor

etpinard Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

🔪 debug cruft

fa78376

etpinard reviewed Jun 9, 2017

View reviewed changes

test axis constraints (range & domain) with log and category axes

67ea3a8

alexcjohnson merged commit 51a479b into master Jun 9, 2017

alexcjohnson deleted the constrain-domain branch June 9, 2017 16:02

etpinard mentioned this pull request Jan 22, 2019

Allow fixedrange axes to scaleanchor with constrain:domain #3460

Closed

Constrain axes by domain #1767

Constrain axes by domain #1767

Conversation

alexcjohnson commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jun 8, 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

alexcjohnson commented Jun 8, 2017

etpinard commented Jun 8, 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

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 Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 9, 2017

etpinard Jun 8, 2017 •

edited

Loading

etpinard commented Jun 8, 2017 •

edited

Loading

etpinard Jun 9, 2017 •

edited

Loading