Sanitize orphan subplot logic and 'last trace' deletion #268

etpinard · 2016-02-19T23:47:21Z

This is a very important PR. So might as well bring in some other folks that should understand its implications ( @cldougl @cpsievert @chriddyp @bpostlethwaite ).

What this PR fixes

Plotly.deleteTraces when applied to the last trace of a subplot now properly deletes that last trace but keeps its axes / scene / geo on the plot (see Plotly.deleteTraces bug #73 for more info)
Plotly.deleteTraces now works on heatmap and contour traces (yep, that didn't work before)
Plotly.deleteTraces now properly delete color bars associated with deleted traces
Plotly.deleteTraces now works on geo traces

What concepts does this PR put forward

(1) plots can have orphan subplots (i.e. subplot without data attached to them), e.g.:

data: [],
layout: {
  xaxis: {},
  yaxis: {}
}

renders a blank set of axes (internally it is now considered _hasCartesian true)

and

data: [],
layout: {
  scene: {}
}

renders a blank scenes (internally _hasGL3D true) and similarly for { geo: {} } layouts.

(2) trace-referenced subplots generate layout subplots for free e.g.

data: [{ type: 'scatter3d', scene: 'scene' }],
layout: {}

add a scene the user layout behind scenes in the defaults step. Similar for cartesian and geo plots.

(3) Deleting the last trace of a subplots deletes the trace in the user data, but keeps the its subplot in the user layout. Combine with concept (1) above, deleting the last trace of a subplot via Plotly.deleteTraces keep the subplot on the plot.

(4) To delete a subplot, one must first delete all its traces and then delete the subplot via relayout e.g. Plotly.relayout('graph', 'scene', null);

(5) Plotting

data: [],
layout: {}

draws a blank plot as previously.

- generalizes the 'find subplot' step where we look for subplots in user data and (now also) in layout before filling in the defaults into their 'full' counterparts. - Before, gl3d and geo defaults where also considering data-referenced subplots. Now, orphan (or data-less) subplot are valid.

- make sure that graph with only orphan cartesian are considered 'cartesian' - make sure that axes associated with GL2D traces aren't considered 'cartesian' - by making layout with { xaxis: {}, yaxis: {} } a 'hasCartesian' plot, deleting the last cartesian trace retains the axes.

- data-only subplot get a corresponding layout subplot 'for free' - plots with orphan scenes/geos are considered HasGL3D and hasGeo respectively.

- by keeping track of traces in each geo from call to call, - make sure that its module's plot method is called so that it is properly removed from the DOM.

- ensures the deleted geos are only properly removed from the DOM. - ensures the deleted contour, heatmap and colorbar are properly removed

etpinard · 2016-02-19T23:49:14Z

TODO:

Plotly.relayout('graph', { 'xaxis': null, 'yaxis': null }); is falling. Fixed in a2f2ce8
Figure out what to do with axis-referenced annotations and shapes. Should they generate cartesian axes for free? Answer: not at the moment. The xref and yref default logic is such that values linked to non-existent axes are coerced to 'paper', which is sensible default.
deleting pie traces does not work. Fixed in 245f8a4

etpinard · 2016-02-19T23:51:23Z

src/plots/cartesian/layout_defaults.js

+    // if gl3d or geo is present on graph. This is retain backward compatible.
+    //
+    // TODO drop this in version 2.0
+    var ignoreOrphan = (layoutOut._hasGL3D || layoutOut._hasGeo);


This is a very important point, to preserve backward compatibility.

... because some old gl3d and geo plots have layout.xaxis and/or layout.yaxis in them, which would generate blank cartesian axes without the line above.

This is presumably just an issue just for plots made and stored in the workspace right? Can we confirm that we're not generating this situation anymore? (Or perhaps we are until we properly handle type changes in restyle) And then how hard would it be to write a command to find and clear these orphans out of stored plots, so we never have to put this logic in? Otherwise I don't see how we're going to be able to take it out later.

I suppose we can put this logic in now, deploy it, then at some later time make and run a cleaning command, then come in after THAT and delete this logic... I just worry that if we don't do it now it will become harder later, and therefore will never happen, and this quirk will live forever.

an we confirm that we're not generating this situation anymore?

scatter3d traces are still generated with a layout.xaxis and layout.yaxis in the workspace.

mdtusz · 2016-02-22T20:00:32Z

test/jasmine/tests/geo_interact_test.js

+                expect(countGeos()).toBe(1);
+                expect(countColorBars()).toBe(1);
+
+                Plotly.deleteTraces(gd, [0]).then(function() {


Can we flatten these out to make use of chaining on the promises?

ah you got me. I was just being lazy.

mdtusz · 2016-02-23T17:39:19Z

test/jasmine/tests/plot_interact_test.js

+                Plotly.relayout(gd, {xaxis: null, yaxis: null}).then(function() {
+                    expect(countScatterTraces()).toEqual(1);
+                    expect(countSubplots()).toEqual(1);
+                    expect(gd.layout.xaxis.range).toBeCloseToArray([-4.79980, 74.48580], 4);


So handy. 🎉

mdtusz · 2016-02-23T17:40:05Z

💃

alexcjohnson · 2016-02-23T18:24:10Z

Just chatting with @etpinard - one more thing popped up: With the orphan logic, if you change trace type and thereby change subplot type (say heatmap -> surface and back again) you'll keep both subplots. @etpinard points out that restyle is the right place to delete the extra subplot when this happens.

remove the extra subplot when restyling empties one out

etpinard · 2016-02-26T19:34:35Z

Closed in favour of #289.

etpinard added 11 commits February 19, 2016 17:58

make Plotly.deleteTraces on 'surface' and 'mesh' traces work

3198229

use findSubplotIds in gl3d and geo defaults steps:

a289fd6

- data-only subplot get a corresponding layout subplot 'for free' - plots with orphan scenes/geos are considered HasGL3D and hasGeo respectively.

rm unnecessary hasCartesian check

41a2a8d

ensure that Plots.getSubplotIds returns an ordered list of ids

ef7f501

make removing geo trace work:

0a6991e

- by keeping track of traces in each geo from call to call, - make sure that its module's plot method is called so that it is properly removed from the DOM.

generalize cleanScenes step:

b7f9471

- ensures the deleted geos are only properly removed from the DOM. - ensures the deleted contour, heatmap and colorbar are properly removed

add geo and cartesian delete traces tests

6ea30d5

add blank plot types test mock

2a32131

lint

f3e1c2b

etpinard added this to the subplotly.js milestone Feb 19, 2016

etpinard mentioned this pull request Feb 19, 2016

Plotly.deleteTraces bug #73

Closed

etpinard reviewed Feb 19, 2016
View reviewed changes

fix typos

dc72d33

mdtusz reviewed Feb 22, 2016
View reviewed changes

etpinard added 6 commits February 22, 2016 18:39

write promise sequence as promise chains

bf6793e

lint

09de224

assure that re-plots changing subplots pass through make framework

a2f2ce8

assure that clearing cartesian axes in relayout lead to a re-calc

ba6eeea

Merge branch 'master' into delete-last-trace

8fac52d

add tests checking axis ranges after partial deletion

1de0d19

etpinard added the status: reviewable label Feb 23, 2016

mdtusz reviewed Feb 23, 2016
View reviewed changes

add cartesian clean plot step to remove old pie traces

245f8a4

etpinard mentioned this pull request Feb 26, 2016

A slightly less ambitious take on orphan subplots and 'delete last trace' #289

Merged

etpinard removed the status: reviewable label Feb 26, 2016

etpinard closed this Feb 26, 2016

etpinard deleted the delete-last-trace branch February 26, 2016 19:34

etpinard mentioned this pull request Feb 29, 2016

Remaining subplot problems #291

Closed

5 tasks

etpinard mentioned this pull request Oct 26, 2017

Automatically remove unused subplots #2125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize orphan subplot logic and 'last trace' deletion #268

Sanitize orphan subplot logic and 'last trace' deletion #268

etpinard commented Feb 19, 2016

etpinard commented Feb 19, 2016

etpinard Feb 19, 2016

etpinard Feb 19, 2016

alexcjohnson Feb 24, 2016

etpinard Feb 26, 2016

mdtusz Feb 22, 2016

etpinard Feb 22, 2016

mdtusz Feb 23, 2016

mdtusz Feb 23, 2016

mdtusz commented Feb 23, 2016

alexcjohnson commented Feb 23, 2016

etpinard commented Feb 26, 2016

Sanitize orphan subplot logic and 'last trace' deletion #268

Sanitize orphan subplot logic and 'last trace' deletion #268

Conversation

etpinard commented Feb 19, 2016

What this PR fixes

What concepts does this PR put forward

etpinard commented Feb 19, 2016

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

mdtusz commented Feb 23, 2016

alexcjohnson commented Feb 23, 2016

etpinard commented Feb 26, 2016