Carpet plot rebase #1595

rreusser · 2017-04-13T17:17:36Z

This PR breaks up a couple not-really-related or loosely-related commits from carpet plots into separate commits and rebases all of carpet plots into one big commit. Supersedes #1239.

etpinard · 2017-04-13T18:26:03Z

src/plots/gl3d/layout/axis_attributes.js

@@ -14,6 +14,7 @@ var extendFlat = require('../../../lib/extend').extendFlat;


 module.exports = {
+    visible: axesAttrs.visible,


visible for 3D axes doesn't actually works, correct?

I think you had to put it in there so that supplyDefaults doesn't error out.

Yeah, correct. Was contemplating the best option as was working back through that. Correct though, it was just to make them not fail.

Perhaps the right answer is not to remove the attribute and avoid supplying that default in that case.

scene.?axis.visible: false would be a nice option too. So I don't think it's worth changing the logic. Opening a new issue about implementing ?axis.visible in 3D will suffice.

done in #1599

oh wow, nice!

#1599 is now merged in this PR.

etpinard · 2017-04-13T18:36:11Z

PR of the year candidate 🏆

rreusser · 2017-04-13T18:37:03Z

alexcjohnson · 2017-04-13T18:38:03Z

src/lib/ensure_array.js

+
+    // If too long, truncate. (If too short, it will grow
+    // automatically so we don't care about that case)
+    out.length = n;


huh, never knew you could resize this way!

Best I can tell, it's actually robust and supported across platforms. It's possible it does weird things internally (like not actually de-allocating that memory. I saw a while ago some discussion of the various internal string apis and string data structures in js. Hint: it's not straightforward). The main goal was to just overwrite the carpet spline stuff rather than allocating everything from scratch every time since that adds up to lots of garbage collection.

From https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/length

The array length property sets or returns the number of elements in an array.

That's right, [..] sets or returns [..].

It's not a read-only property like one might assume.

(best-case scenario though is that it actually truncates in-place without actually copying. I mean maybe copies, but slice() definitely copies.)

alexcjohnson · 2017-04-13T18:55:22Z

src/components/drawing/index.js

+    var bbox = drawing.bBox(dummyText.node());
+    dummyText.remove();
+    return bbox;
+};


I see why you did it this way, but it seems a little redundant, we're making a new node only to clone it in the global tester, then we delete them both. If this ends up getting called a lot, for performance we may want to break up drawing.bBox in such a way that this element can get created in the global tester in the first place.

That makes sense. I didn't appreciate that the tester was also cloning it. And to add to that, it looks like I only ended up using it in one place. 😕

(but on the plus side, to answer your question, at least it doesn't end up getting called a lot 😄 )

etpinard · 2017-04-13T19:53:06Z

test/jasmine/tests/carpet_test.js

+        expect(traceOut.db).toBeUndefined();
+        expect(traceOut.a0).toBeUndefined();
+        expect(traceOut.b0).toBeUndefined();
+        expect(traceOut.visible).toBe(true);


@rreusser looks like only this assertion fails when removing the if(!test.supplyDefaults) return; statement above.

by the way @rreusser I'm adding a few restyle test cases on a branch, so I can take care of ⤴️ if you like.

If you're already editing it, then 👍 . I think most of the carpet tests should actually be moved out of this file since a lot of the functions being tested are better suited for lib.

done in c98553a

- and replace it by Lib.extendFlat({}, /* */)

- no need to coerce attribute that don't have an effect

A few more carpet plot tests

Axis visible in 3D

etpinard · 2017-04-17T13:53:21Z

@rreusser a well-earned 💃

Not sure if @alexcjohnson 's #1595 (comment) is blocking though.

🎉 🚀 🚀 🚀 🚀 🚀 🚀 🚀 🚀 🎉

alexcjohnson · 2017-04-17T14:48:03Z

Not sure if @alexcjohnson 's #1595 (comment) is blocking though.

Nah, if it only gets sparing use it's not blocking. Awesome work @rreusser & @etpinard !

rreusser · 2017-04-17T14:53:09Z

Removed nonetheless. You're correct that it really wasn't needed there and wasn't used elsewhere. 👍

alexcjohnson · 2017-09-01T23:54:49Z

src/traces/carpet/attributes.js

+    baxis: extendFlat({}, axisAttrs),
+    font: {
+        family: extendFlat({}, fontAttrs.family, {
+            dflt: '"Open Sans", verdana, arial, sans-serif'


@rreusser a little late for me to notice this, but is there a reason this font gets its own defaults? Is it too late to make this inherit from the global layout.font? I see that the various axis fonts inherit from this, which is great.
cc @etpinard

Ah, good point. Yes, I valid to make the switch now. Though it's technically a change, I think the number of actual breakages would be outweighed by API consistency.

Great - I just noticed some of these things as I'm pushing more of the restyle/relayout logic into the schema... I'll take care of these after I'm done with that.

Is it too late to make this inherit from the global layout.font?

Nop, not too late. I'd call this a bug fix.

alexcjohnson · 2017-09-02T00:04:28Z

src/traces/carpet/axis_attributes.js

+        ].join(' ')
+    },
+
+    fixedrange: {


@rreusser does fixedrange do anything on a carpet axis? Can I remove it?

rreusser added 13 commits April 13, 2017 13:09

Add ensure array lib function to allocate/resize

eee837f

Add axis cartesian axis visibility option

5a0b788

Add measureText function to drawing

89e095e

Tweak showscale to allow unset (instead of strictly false)

e1a0417

Contour plot lines to use non-scaling stroke

620cd43

Genericize heatmap xyz variable names

728a6f4

More carefully track legend trace isolation indices

c2994e7

Add optional overrides to contour handleStyleDefaults

fe9850c

Genericize tolerances in find_all_paths to make way for carpet

d1f2258

Add flag to disable scatter marker culling (needed for carpet)

4bcf438

Tweak contour colorscale logic since carpet may set infinities

6b896d8

Add group to plot

b8c71e1

Carpet plots

e4971ef

rreusser mentioned this pull request Apr 13, 2017

[WIP] Carpet plots #1239

Closed

44 tasks

etpinard added this to the v1.26.0 milestone Apr 13, 2017

etpinard added status: reviewable feature something new labels Apr 13, 2017

etpinard reviewed Apr 13, 2017

View reviewed changes

alexcjohnson reviewed Apr 13, 2017

View reviewed changes

etpinard reviewed Apr 13, 2017

View reviewed changes

etpinard added 2 commits April 13, 2017 16:34

make rreusser's carpet tests pass

c98553a

add a few carpet interaction tests

71b96f3

etpinard mentioned this pull request Apr 13, 2017

A few more carpet plot tests #1596

Merged

etpinard added 4 commits April 13, 2017 22:26

🔪 arraytools dep (sorry @bpostlethwaite)

bc1c5c8

- and replace it by Lib.extendFlat({}, /* */)

exit early in axis defaults when visible is false

9c76d7a

- no need to coerce attribute that don't have an effect

add axis visible: false mock

85d6b8a

implement axis visible in 3D

54f092d

add 3d axis visible: false mock

d9cb011

etpinard mentioned this pull request Apr 14, 2017

Axis visible in 3D #1599

Merged

etpinard added 3 commits April 17, 2017 09:20

Merge pull request #1596 from plotly/carpet-test-etienne

f1e8aee

A few more carpet plot tests

extendFlat -> slice

bc0f890

Merge pull request #1599 from plotly/axis-visible-3d

c2a7092

Axis visible in 3D

rreusser merged commit ba0e3a5 into master Apr 17, 2017

rreusser deleted the carpet-rebase branch April 17, 2017 15:17

etpinard mentioned this pull request Jun 28, 2017

Carpet plots #1179

Closed

alexcjohnson reviewed Sep 1, 2017

View reviewed changes

alexcjohnson reviewed Sep 2, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carpet plot rebase #1595

Carpet plot rebase #1595

rreusser commented Apr 13, 2017

etpinard Apr 13, 2017

rreusser Apr 13, 2017

rreusser Apr 13, 2017 •

edited

Loading

etpinard Apr 13, 2017

etpinard Apr 14, 2017

rreusser Apr 14, 2017

etpinard Apr 17, 2017

etpinard commented Apr 13, 2017

rreusser commented Apr 13, 2017

alexcjohnson Apr 13, 2017

rreusser Apr 13, 2017 •

edited

Loading

etpinard Apr 13, 2017 •

edited

Loading

rreusser Apr 13, 2017

alexcjohnson Apr 13, 2017

rreusser Apr 13, 2017 •

edited

Loading

rreusser Apr 13, 2017

etpinard Apr 13, 2017

etpinard Apr 13, 2017

rreusser Apr 13, 2017 •

edited

Loading

etpinard Apr 13, 2017

etpinard commented Apr 17, 2017

alexcjohnson commented Apr 17, 2017

rreusser commented Apr 17, 2017 •

edited

Loading

alexcjohnson Sep 1, 2017

rreusser Sep 2, 2017

alexcjohnson Sep 2, 2017

etpinard Sep 5, 2017

alexcjohnson Sep 2, 2017

rreusser Sep 2, 2017

		@@ -14,6 +14,7 @@ var extendFlat = require('../../../lib/extend').extendFlat;


		module.exports = {
		visible: axesAttrs.visible,

Carpet plot rebase #1595

Carpet plot rebase #1595

Conversation

rreusser commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Apr 13, 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

etpinard commented Apr 13, 2017

rreusser commented Apr 13, 2017

Choose a reason for hiding this comment

rreusser Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

etpinard Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Apr 13, 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

rreusser Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 17, 2017

alexcjohnson commented Apr 17, 2017

rreusser commented Apr 17, 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

rreusser Apr 13, 2017 •

edited

Loading

rreusser Apr 13, 2017 •

edited

Loading

etpinard Apr 13, 2017 •

edited

Loading

rreusser Apr 13, 2017 •

edited

Loading

rreusser Apr 13, 2017 •

edited

Loading

rreusser commented Apr 17, 2017 •

edited

Loading