Color gradient for `scatter3d` snail trail lines [WIP] #617

monfera · 2016-06-08T13:53:19Z

The current PR intends to cover functionality as per #581, i.e. a scatter3d solution.

etpinard · 2016-06-08T14:20:24Z

Looking good so far.

It's probably already on your checklist, but just to be sure: we should support the case of numeric line.color arrays + line.colorscale off this PR.

monfera · 2016-06-08T14:41:12Z

@etpinard yes, the included example uses line.color as an array, and preexisting mocks with a non-array line.color pass the image tests, but test cases for other ways for color specification will be added.

monfera · 2016-06-09T20:41:14Z

@etpinard On scatter3d there's interaction between the line and the marker coloring. Basically, the marker conversion deferred to line related color settings (well... it was limited to one prop in the past as I've just recently added the full-fledged color specification with 7 props, colorscale etc.) in case there was no supplied value for marker (fill) and/or marker.line. Now that we have a lot more props, I'd either need to discontinue marker and marker.line colors kinda defaulting to the line colorspec, or I need to do something cohesive as we have 7 props and they may be explicitly specced or missing in the line part of the traceIn. For implementation simplicity and usage clarity I wouldn't mind the former (I have the old defaulting commented out as a test) but for compatibility or other reasons you probably prefer something like taking over all props from line to marker and marker.line as defaults, or if it proves nontrivial, at least a way of preserving how old plots rendered (i.e. supporting the defaulting for at least the color prop). Can you share your thoughts on this?

etpinard · 2016-06-14T02:44:31Z

@monfera Wow that's a lot of new tests !!!

I doubt that they all add something useful to the table though. Generating gl3d image is pretty expansive. I think we should try to add a little image mocks as possible (while still testing the added features of course). All in an effort to keep the execution time for npm run test-image as bearable as possible. Less-expansive jasmine tests (especially when targeting the defaults in isolation) should be favoured to test small variables in user data.

For example, no need to add an image test for marker.colorscale (that's done already in gl3d_scatter3d-colorscale. Instead of adding multiple test mock for string line.color and numeric line.color, why not try to merge them into a multi-trace graph?

etpinard · 2016-06-14T02:44:52Z

src/components/colorscale/calc.js

@@ -20,7 +20,7 @@ module.exports = function calc(trace, vals, containerStr, cLetter) {

    if(containerStr) {
        container = Lib.nestedProperty(trace, containerStr).get();
-        inputContainer = Lib.nestedProperty(trace._input, containerStr).get();
+        inputContainer = Lib.nestedProperty(trace, containerStr).get();


@etpinard it was just a stop-gap. A few lines below the preexisting code set cmin/cmax/colorscale properties on the inputContainer. My understanding is that we not set or overwrite what the user gaveth us; that we'd decorate the output container with defaults (which was also in the code). I eliminated lines that mutated inputContainer, but glad to do something else if I'm trampling over some important aspect unknowingly.

My understanding is that we not set or overwrite what the user gaveth us;

Correct. But there are a few exceptions to the rule. Namely to make restyle work as desired. Too bad we never added any test cases. But, something like

Plotly.plot(/* some graph with a colorscale and cmin/cmax auto filled */) Plotly.restyle(gd, 'cauto', false);

should return the original graph, and at the moment to only way to do so is to mutate gd.data .

This is also the case for (all I think) other attributes with an auto flag e.g. histogram xbins, xaxis.range ...

What we should do is: mark those attributes with a special flag in the attributes.js and handle them separately in restyle / relayout so that we don't have to mutate gd.data. But, that will be for a future PR.

Interesting. Thanks for pointing this out. I'll take a look at this issue tomorrow am.

What we should do is: mark those attributes with a special flag in the attributes.js

I see what you mean, wondered why marker.cauto restyle magically worked and line.cauto didn't. Sang a mantra of relief when I found it :-) https://github.com/plotly/plotly.js/blob/master/src/plot_api/plot_api.js#L1559-L1594

I don't remember one off the top of my head.

I switched the code as discussed. But it seems that if we persist the autocalculated cmin/cmax then it does not do what you want (provided I understand it well). Why? Only # 5 differs from my above list, the rest is copied.

Initially the plot has specific cmin/cmax values; cauto false

I do Plotly.restyle(gd, 'marker.cauto', true)

Result is, the color palette now expands to the entire range of the data as it should

I do Plotly.restyle(gd, 'marker.cauto', false)

Result is, the colors on the markers do not get restored to what they were because step 2 caused a destructive, irrecoverable mutation of the original user input

I reverted to my simpler code for now, haven't pushed the code snippet in the above comment but open to do whatever our clarification leads to.

backward-incompatible change

When you mentioned this reliance I initially also thought that retaining compatibility goes without saying. But now there's this nagging feeling that maybe it's not a big risk to break compat for this specifically, and perhaps see if plot.ly/plot workspace needs fixing and handle reported issues in the (perhaps) unlikely case of user reliance. In general I'd favor retaining original user input and not sharing our own intermediate work calculations unless through a querying API (rather than directly querying objects which are implementation detail). As you have a lot more info on the consequences, of course its your call but wanted to share a base sentiment.

The technical difficulty is that this object, on which it attempts to set cmin/cmax may not exist:

In what scenario? calcColorscale is only called if hasMarkers(trace) is true (and if hasLines(trace), no ?

@monfera I really can't see a reason for calcCalcscale to be modified in this PR.

Could you elaborate?

@etpinard

In what scenario? calcColorscale is only called if hasMarkers(trace) is true (and if hasLines(trace), no ?

From a previous comment:

The technical difficulty is that this object, on which it attempts to set cmin/cmax may not exist: marker inherits props to marker.line but marker.line might not have been specified among the input. Therefore the code attempts to set cmin/cmax on undefined.

I really can't see a reason for calcCalcscale to be modified in this PR. Could you elaborate?

In the past, the color determination was much simpler for scatter3d: its lines, markers did not participate in the sophisticated color specification that has the attributes from color_attributes.js. Therefore this specific piece of code was not exercised for scatter3d lines, markers and marker lines. This explains why there was no issue with cmin/cmax here (the code didn't get called and these props weren't present for scatter3d anyway).

It looked sensible to use common code (colorscale/calc.js) to deal with the common color attribute set (color_attributes.js). Therefore colorscale/calc.js got exercised in new ways, under which it failed (in my understanding) for the following two reasons (not new, just a recap of previous discussion):

It errors out because it tries to set attributes on an object that may not exist (e.g. marker.line)

Persisting a calculated minimum/maximum value on the input (specifically, trace._input) means that if the user sets cauto to true, then back to false, then they will get a different result than what they originally had (cmin/cmax would get overwritten by the data-drivem minimum/maximum).

So I thought we should use this piece of code for the newly introduced scatter3d coloring options for line and marker rather than duplicating this file and it implies a change. Alternatively, if you'd like, I can copy this file just for scatter3d and contidionally invoke that. Another alternative is if I use the above snippet to solve the case of missing object, so that the tests pass, but in this case, as mentioned, toggling cauto back and forth will override the original user-supplied cmin/cmax values.

etpinard · 2016-06-14T02:52:06Z

@monfera Looks like you got all the functionality in. Fantastic 🎉

This will make a great addition once the tests are cleaned up.

monfera · 2016-06-15T09:42:18Z

src/plot_api/plot_api.js

@@ -1586,7 +1586,9 @@ Plotly.restyle = function restyle(gd, astr, val, traces) {
        'outsidetextfont.size', 'outsidetextfont.family', 'outsidetextfont.color',
        'hole', 'scalegroup', 'domain', 'domain.x', 'domain.y',
        'domain.x[0]', 'domain.x[1]', 'domain.y[0]', 'domain.y[1]',
-        'tilt', 'tiltaxis', 'depth', 'direction', 'rotation', 'pull'
+        'tilt', 'tiltaxis', 'depth', 'direction', 'rotation', 'pull',


@etpinard just thinking about how these arrays could simply be done by flags in the attribute.js definitions but it's not trivial due to the irregularities:

Sometimes (as domain.x[0] here) the check is below the prop level (this is easy to cover though)

There are multiple sets: recalcAttrs, autorangeAttrs, replotAttrs, 'swapAttrs' (still easy to cover but not pretty)

Lots of manual overrides (see elseif tree with doextras: https://github.com/plotly/plotly.js/blob/master/src/plot_api/plot_api.js#L1736-L1814

Innumerable special casing for a wide diversity of things such as pie, LAYOUT, if(ai === 'orientationaxes') etc.

It's possible to first cover 1, 2 and maybe 3 but even that leaves quite a lot of entanglement.

etpinard · 2016-06-15T15:31:00Z

src/traces/scatter/colorscale_calc.js

-        calcColorscale(trace, marker.color, 'marker', 'c');
+
+    if(subTypes.hasLines(trace) && hasColorscale(trace, 'line')) {
+        calcColorscale(trace, trace.line.color, 'line', 'c');


nicely done here.

liuyifang · 2016-06-20T10:11:22Z

Hi @monfera
I am a plotly pro user and using the scatter3d function to build a scientific website like this: http://3dgb.cbi.pku.edu.cn/disease/details/826/. I need to add the gradient color to the 3D structure and find out your work is definitely what I need.
Can you give me some advice about how to use your latest version of plotly, I need to use this new function in a hurry. Thanks~

monfera · 2016-06-20T11:41:27Z

Hello @liuyifang you have a great website, very interesting!

This branch needs a couple of smaller decisions and test simplifications, and will lead to gradient line coloring, an example of which can be seen on the request ticket: #581 (comment)

Ideally this can be merged in a day or two, and a subsequent release in another few days, but depends on workload on tasks running in parallel at Plotly. In this branch, the color interpolation is between connected points, i.e. the color is interpolated via a default linear RGB from the beginning to end of a specific line section. While in theory it's possible to check out this branch and use this, the way it's configured, and the way the default colors are determined will still change a bit, so I think it would be best to wait for a released bundle.

As a separate feature, we're also working on an initial version of streamtubes that has support for lighting, occlusion, varying radius, Catmull-Rom spline based curvature and interpolation, and other features, some examples (feature list and target date aren't final yet):

Naturally these types of plots require more processing and your site may not require these features, though the color interpolation will be more flexible in this version too (palette based such as Viridis, rainbow).

etpinard · 2016-06-21T15:54:14Z

Fantastic work. 🍻

etpinard added feature something new status: in progress labels Jun 8, 2016

monfera force-pushed the 581-scatter3d-line-color branch 7 times, most recently from ef83a6b to 46104ac Compare June 13, 2016 18:11

etpinard reviewed Jun 14, 2016
View reviewed changes

monfera reviewed Jun 15, 2016
View reviewed changes

This was referenced Jun 15, 2016

Thinking about improving data updates #647

Closed

Move property literals out of Plotly.restyle #648

Closed

etpinard reviewed Jun 15, 2016
View reviewed changes

monfera force-pushed the 581-scatter3d-line-color branch from 6b94edd to cd32fe4 Compare June 20, 2016 14:13

monfera force-pushed the 581-scatter3d-line-color branch 2 times, most recently from 01634e5 to 6bab94f Compare June 21, 2016 12:16

monfera added 5 commits June 21, 2016 17:22

581 color attribute doc updates

4549f4f

581 scatter3d line color attribute specified

64344a2

581 generalizing between line and marker color scale

c51d719

581 line coloring defaults and conversions

1ae1932

581 line coloring tests

7347847

monfera force-pushed the 581-scatter3d-line-color branch from 70ba46d to 7347847 Compare June 21, 2016 15:32

etpinard added status: reviewable and removed status: in progress labels Jun 21, 2016

etpinard mentioned this pull request Jun 21, 2016

Colorspec doc corrections and clarifications #625

Closed

etpinard merged commit f69fc5f into plotly:master Jun 21, 2016

monfera mentioned this pull request Oct 11, 2016

Line colorscale on scatter graph #581

Closed

etpinard mentioned this pull request Aug 1, 2018

scatter3d.line.showscale doesn't show a colorscale #2862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color gradient for `scatter3d` snail trail lines [WIP] #617

Color gradient for `scatter3d` snail trail lines [WIP] #617

monfera commented Jun 8, 2016 •

edited

Loading

etpinard commented Jun 8, 2016

monfera commented Jun 8, 2016

monfera commented Jun 9, 2016

etpinard commented Jun 14, 2016 •

edited

Loading

etpinard Jun 14, 2016

monfera Jun 14, 2016

etpinard Jun 14, 2016

etpinard Jun 14, 2016 •

edited

Loading

etpinard Jun 14, 2016 •

edited

Loading

etpinard Jun 14, 2016

monfera Jun 15, 2016

etpinard Jun 15, 2016 •

edited

Loading

etpinard Jun 15, 2016

monfera Jun 15, 2016 •

edited

Loading

etpinard commented Jun 14, 2016

monfera Jun 15, 2016

etpinard Jun 15, 2016

liuyifang commented Jun 20, 2016

monfera commented Jun 20, 2016 •

edited

Loading

etpinard commented Jun 21, 2016

Color gradient for scatter3d snail trail lines [WIP] #617

Color gradient for scatter3d snail trail lines [WIP] #617

Conversation

monfera commented Jun 8, 2016 • edited Loading

etpinard commented Jun 8, 2016

monfera commented Jun 8, 2016

monfera commented Jun 9, 2016

etpinard commented Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jun 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera Jun 15, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyifang commented Jun 20, 2016

monfera commented Jun 20, 2016 • edited Loading

etpinard commented Jun 21, 2016

Color gradient for `scatter3d` snail trail lines [WIP] #617

Color gradient for `scatter3d` snail trail lines [WIP] #617

monfera commented Jun 8, 2016 •

edited

Loading

etpinard commented Jun 14, 2016 •

edited

Loading

etpinard Jun 14, 2016 •

edited

Loading

etpinard Jun 14, 2016 •

edited

Loading

etpinard Jun 15, 2016 •

edited

Loading

monfera Jun 15, 2016 •

edited

Loading

monfera commented Jun 20, 2016 •

edited

Loading