Sankey: colorscales per component, linked to concentration #3501

antoinerg · 2019-01-31T04:33:42Z

An attempt at closing #3318

antoinerg · 2019-02-14T22:50:58Z

Here's a Codepen showcasing the new feature introduced in this PR: https://codepen.io/antoinerg/pen/qgJbOr . Hovering on links will display the new computed statistics.

By default in Sankey, when we hover on a link, its opacity is reduced. I think this looks odd in the Codepen above.

@alexcjohnson what do you think?

etpinard · 2019-02-14T22:55:36Z

src/traces/sankey/render.js

@@ -10,6 +10,7 @@

 var c = require('./constants');
 var d3 = require('d3');
+var sum = require('d3-array').sum;


Ha this here only works between d3-sankey already depends on d3-array.

We should add d3-array to the (plotly.js) dependencies OR use plain loops and save one line in our package.json.

I think I will do the latter and save one line in our package.json.

I should probably replace all the forEach while I'm at it right?

I should probably replace all the forEach while I'm at it right?

That would be amazing 💪

Done in c228b37

alexcjohnson · 2019-02-15T01:53:23Z

Hovering on links will display the new computed statistics.

Nice - perhaps we can include the concentration in the label by default, for links that have a concentration colorscale?

By default in Sankey, when we hover on a link, its opacity is reduced. I think this looks odd in the Codepen above.

That's weird, normally opacity is increased on hover - is that because normally the opacity is low but here its high, and on hover it gets some set value that's more than the default but less than we have here?

Anyway it seems like we probably don't want an opacity change at all when the color is displaying quantitative information via a colorscale - especially since scales like you've used, with white at one end, will probably be common, and these scales are equivalent to opacity. But nor do we want to decrease opacity on hover even for non-colorscaled links. Perhaps we just need to find a different way to highlight the hovered links if their opacity is above some threshold and/or they have a colorscale. Perhaps adding, thickening, or increasing the contrast of the link outline?

By the same token, we probably want the hover label to be fully opaque when it's describing a colorscaled link, so it will have the same color as the link.

antoinerg · 2019-02-15T19:34:25Z

Nice - perhaps we can include the concentration in the label by default, for links that have a concentration colorscale?

Done in c228b37

That's weird, normally opacity is increased on hover - is that because normally the opacity is low but here its high, and on hover it gets some set value that's more than the default but less than we have here?

Yes you are right: it used to simply set the opacity of 0.4 which is higher than the default but less than we had here.

Perhaps we just need to find a different way to highlight the hovered links if their opacity is above some threshold and/or they have a colorscale. Perhaps adding, thickening, or increasing the contrast of the link outline?

In c228b37, I simply removed hover effects for links with a concentration colorscale. @etpinard Do you have any suggestion of what to do to highlight the hovered links? Is there any precedent in plotly.js for such a situation? Displaying a link outline seems like a good idea but I wanted to double-check with you first.

By the same token, we probably want the hover label to be fully opaque when it's describing a colorscaled link, so it will have the same color as the link.

Done in c228b37

etpinard · 2019-02-15T19:59:14Z

@etpinard Do you have any suggestion of what to do to highlight the hovered links? Is there any precedent in plotly.js for such a situation?

Do you really "need" to highlight them?

their positioning and coloring makes the hover labels pretty unambiguous to me, at least in this scenario.

antoinerg · 2019-02-15T20:07:15Z

Do you really "need" to highlight them?

@etpinard I was wondering the exact same thing. I'd be curious to have @alexcjohnson's opinion on this one!

etpinard · 2019-02-15T20:08:29Z

I'd be curious to have @alexcjohnson's opinion on this one!

Good call. I'm voting no.

alexcjohnson · 2019-02-15T20:50:56Z

That's a pretty simple diagram, but in more complicated ones like the energy flows example a highlight can really help to see clearly what nodes the link connects.

I'd be OK with omitting highlighting for now when the link is opaque, but we should keep it in the default case and reserve the right to add something like outlining later on.

antoinerg · 2019-02-15T21:27:25Z

@etpinard It seems like @alexcjohnson is OK with not highlighting colorscaled links (at least for now). Nothing precludes us from changing their appearance later on so I'm comfortable with the status quo.

I agree that the existing appearance of hovered non-colorscaled links is useful and I confirm that it is still intact.

As of right now, I'm not testing in jasmine that the flow statistics are properly computed although the mock effectively 🔒 this down. Let me know if that is sufficient.

etpinard · 2019-02-18T15:14:43Z

package-lock.json

@@ -4142,8 +4142,7 @@
        "ansi-regex": {
          "version": "2.1.1",
          "bundled": true,
-          "dev": true,


@antoinerg which version of npm are you using?

Your PR doesn't change any of our dependencies, so it shouldn't change the package-lock. Can you revert these changes?

I now updated to npm 6.8.0, ran npm i and the package-lock.json has now zero changes! Sorry about that... I need a safety check on my dev machine.

Done f628b54

etpinard · 2019-02-18T15:16:48Z

src/traces/sankey/render.js

@@ -616,6 +686,7 @@ module.exports = function(svg, calcData, layout, callbacks) {
          .attr('d', linkPath())
          .call(attachPointerEvents, sankey, callbacks.linkEvents);

+


⚡ no need to commit this newline.

Done in f628b54

etpinard · 2019-02-18T15:17:33Z

Nicely done @antoinerg !

I made two minor comments. This thing is set to be part of 1.45.0!

etpinard · 2019-02-18T17:47:38Z

Nicely done. 💃

identify link per label, compute concentration and color accordingly

6f056eb

antoinerg added feature something new status: in progress labels Jan 31, 2019

sankey: fix attribute role and editType

5e43023

antoinerg changed the base branch from sankey-circular to master February 14, 2019 22:04

etpinard reviewed Feb 14, 2019

View reviewed changes

antoinerg added 3 commits February 15, 2019 10:48

Merge branch 'master' into pr-sankey-link-concentration

a791291

sankey: forEach -> for loops, no FX on link hover if concentrationscale

c228b37

remove unused d3-array from Sankey

c976dbb

antoinerg added status: reviewable and removed status: in progress labels Feb 15, 2019

sankey: test that eventData has appropriate flow statistics keys

ca6661c

antoinerg removed the status: discussion needed label Feb 15, 2019

fix typo in mock's figure title

60d0448

etpinard added this to the v1.45.0 milestone Feb 18, 2019

etpinard reviewed Feb 18, 2019

View reviewed changes

update packge-lock.json, remove newline

f628b54

antoinerg merged commit 12097a2 into master Feb 18, 2019

antoinerg deleted the pr-sankey-link-concentration branch February 18, 2019 17:50

antoinerg mentioned this pull request Feb 18, 2019

Sankey: colorscales per component, linked to concentration #3318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sankey: colorscales per component, linked to concentration #3501

Sankey: colorscales per component, linked to concentration #3501

antoinerg commented Jan 31, 2019 •

edited

Loading

antoinerg commented Feb 14, 2019 •

edited

Loading

etpinard Feb 14, 2019

antoinerg Feb 14, 2019

etpinard Feb 15, 2019

antoinerg Feb 15, 2019

alexcjohnson commented Feb 15, 2019

antoinerg commented Feb 15, 2019 •

edited

Loading

etpinard commented Feb 15, 2019 •

edited

Loading

antoinerg commented Feb 15, 2019

etpinard commented Feb 15, 2019

alexcjohnson commented Feb 15, 2019

antoinerg commented Feb 15, 2019

etpinard Feb 18, 2019

antoinerg Feb 18, 2019

etpinard Feb 18, 2019

antoinerg Feb 18, 2019

etpinard commented Feb 18, 2019

etpinard commented Feb 18, 2019

		@@ -616,6 +686,7 @@ module.exports = function(svg, calcData, layout, callbacks) {
		.attr('d', linkPath())
		.call(attachPointerEvents, sankey, callbacks.linkEvents);

Sankey: colorscales per component, linked to concentration #3501

Sankey: colorscales per component, linked to concentration #3501

Conversation

antoinerg commented Jan 31, 2019 • edited Loading

antoinerg commented Feb 14, 2019 • 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 Feb 15, 2019

antoinerg commented Feb 15, 2019 • edited Loading

etpinard commented Feb 15, 2019 • edited Loading

antoinerg commented Feb 15, 2019

etpinard commented Feb 15, 2019

alexcjohnson commented Feb 15, 2019

antoinerg commented Feb 15, 2019

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 Feb 18, 2019

etpinard commented Feb 18, 2019

antoinerg commented Jan 31, 2019 •

edited

Loading

antoinerg commented Feb 14, 2019 •

edited

Loading

antoinerg commented Feb 15, 2019 •

edited

Loading

etpinard commented Feb 15, 2019 •

edited

Loading