Icicle Trace #5546

Kully · 2021-03-11T19:12:25Z

closes: #5369

Overview of the Pull Request:

This PR is for the purposes of adding an Icicle/Flame charts to the Plotly traces. The high level aim is for the Icicle to achieve parity with the sunburst and treemap traces, and other features

Original Proposal GIST -> here
Reference Icicle branch (author: @archmoj) -> here (do not modify)

Setting up the PR

git rebase their local branch off the latest master,
make sure to not git add the dist/ folder (the dist/ is updated only on version bumps),
make sure to commit changes to the package-lock.json file (if any new dependency required),
write an overview of what the PR attempts to do,
select the Allow edits from maintainers option (see this article for more details).

List of Tasks

Icicle.Bug.-.Root.is.Clickable.mov

…icle

…entations

…icle

…entations

Kully · 2021-03-11T20:27:44Z

Bug - No transition on slicetext

While playing around with the Icicle chart, I found a bug:

Ensure that Icicle has transition enabled
Click on a slice or pathbar so as to pan the camera to reveal slices that are not in the layout.
Finally, notice that the slicetrace is already fixed into place and they do not transition.

icicle.-.text.no.transition.mov

[Notice that the Awan slice is the only one that has no transition. It's also the only one that has children. All the other slices seem to behave normally]

Looking deeper into this, I found that this behaviour is most likely a by-product of the way the transition logic is coded for all of Sunburst, Treemap, and now Icicle:

Sunburst: Notice how Thurs comes into view not by stretching and sheering, but is already where it needs to be, with scale being the main param for its transition.

sunburst_2.mov

Insights/Resolutions

I was exploring the makeUpdateTextInterpolator function but was not able to find anything relevant.

Do any of you have insights into what the reason/problem could be?

CC
@nicolaskruchten @alexcjohnson

Maybe the path of least resistance is to only make text visible once the slides animate into place, kinda like how treemap works sometimes:

treemap.-.text.appears.mov

nicolaskruchten · 2021-03-11T21:50:20Z

Re the transition bug, if it already impacts treemap/sunburst, let's just leave it as-is here and log an issue to cover the existing ones and then loop back around once this PR is merged to tackle it for all three trace types.

Kully · 2021-03-12T17:39:20Z

Re the transition bug, if it already impacts treemap/sunburst, let's just leave it as-is here and log an issue to cover the existing ones and then loop back around once this PR is merged to tackle it for all three trace types.

That sounds like a solid plan. Will do that.

src/traces/icicle/plot.js

archmoj · 2021-03-16T13:17:07Z

Instead of adding 4 mocks added 1477ceb, can't we add a simple & single mock to test 4 directions?
You also need to generate baselines (see here) for new mocks and validate them in test/jasmine/tests/mock_test.js.
I also suggest you start test/jasmine/tests/icicle_test.js and copy and adapt good portion of treemap and sunburst tests, e.g. supply defaults, calc and other tests sooner than later.
Thanks!

…cle-trace-base merge from master

Kully · 2021-03-16T16:13:44Z

Instead of adding 4 mocks added 1477ceb, can't we add a simple & single mock to test 4 directions?

Great point, I'll reconfigure this. Thanks for the tip.

Here's what the all-directions mock looks like now
last updated Mar 17, 2021

… into icicle-trace-base pull from remote branch

…ss the trace domain anymore

archmoj · 2021-05-12T15:33:21Z

After #5653 there is no need for the mock_test.js file.
@Kully would you please merge origin/master into this branch?
Thanks!

Kully · 2021-05-19T18:31:17Z

@Kully would you please merge origin/master into this branch?

Okay this should be good to go now.

archmoj · 2021-05-19T19:01:12Z

@Kully now in order for the image test pass please replace test_images/icicle_values_colorscale.png.
See: https://app.circleci.com/pipelines/github/plotly/plotly.js/3891/workflows/8b0c97f3-69ff-4f8c-a264-1e293a46d8f3/jobs/113611/artifacts

The diff is in respect to #5611.

Kully · 2021-05-31T04:41:11Z

now in order for the image test pass please replace test_images/icicle_values_colorscale.png.

Looks like you already got to it in your commit, thank you 🙂

Is there anything else for us to do now? I can't wait for these ICICLES to hit the main library.

src/traces/icicle/partition.js

archmoj · 2021-06-11T18:45:11Z

These repetitive string comparisons could be slow.
Lets add
var isIcicle = type === 'icicle';
to the top and then simply use isIcicle here and below e.g.
x0 += (isIcicle ? pad : pad.l)  - TEXTPAD;

Addressed in c57dda5.

nicolaskruchten · 2021-06-15T14:00:21Z

src/traces/icicle/attributes.js

+            editType: 'plot',
+            description: [
+                'Sets the orientation of the icicle.',
+                'With *v* the icicle grows vertically.',


this isn't really clear to me... "v" means "bottom up" and "h" means "left to right" ? It would be helpful to note here that the other two possibilities (top-down and right-to-left) can be achieved via flip if that is indeed how to do it.

In fact I might phrase this as "the root node(s) are on the left" rather than talking about icicles "growing" :)

Great point, and I agree with this. I'l add that note.

nicolaskruchten · 2021-06-15T14:08:17Z

src/traces/icicle/attributes.js

+
+        flip: treemapAttrs.tiling.flip,
+
+        pad: treemapAttrs.tiling.pad,


I think the default value for this attribute should be 0, so as to look more like sunbursts and less like treemaps

Yes that's a good idea as well. I remember when playing around with padding, 0 or close-to-0 looked the best to me.

Will change this.

archmoj · 2021-06-17T17:24:32Z

Thanks very much @Kully and @mtwichan for the hard work 🥇
💃 💃

archmoj and others added 18 commits February 23, 2021 00:21

start icicle trace by duplicating and adjusting treemap code

b3c9c08

can reuse treemp/draw_ancestors and sunburt/style

66c6730

working prototype

b3e1bb9

drop unused treemap props and bring in relevant sunburst props for ic…

0822bf5

…icle

add tiling orientation for icicle

5aa3310

add some icicle mocks and baselines and validate them

46182ad

where possible reuse treemap attributes in icicle

9f4f248

adjust syntax test for icicle children

d5fc016

add(mocks): icicle plots that showcase left, right, top, and down ori…

1477ceb

…entations

start icicle trace by duplicating and adjusting treemap code

4f53fe1

can reuse treemp/draw_ancestors and sunburt/style

f2e0698

working prototype

9c0ad73

drop unused treemap props and bring in relevant sunburst props for ic…

77235b4

…icle

add tiling orientation for icicle

938cf4a

add some icicle mocks and baselines and validate them

9a57346

where possible reuse treemap attributes in icicle

5b5bb5e

adjust syntax test for icicle children

b82fad6

add(mocks): icicle plots that showcase left, right, top, and down ori…

3360d78

…entations

nicolaskruchten marked this pull request as draft March 11, 2021 19:20

archmoj added the type: new trace type label Mar 11, 2021

Kully mentioned this pull request Mar 12, 2021

Broken text transitions for sunburst/treemap charts #5547

Open

archmoj reviewed Mar 16, 2021

View reviewed changes

src/traces/icicle/plot.js Show resolved Hide resolved

Merge branch 'master' of https://github.com/plotly/plotly.js into ici…

e8f63dc

…cle-trace-base merge from master

Coding-with-Adam added 2 commits March 16, 2021 12:31

combine all 4 icicle direction mocks into single mock

4607077

Merge branch 'icicle-trace-base' of https://github.com/plotly/plotly.js…

be5ec77

… into icicle-trace-base pull from remote branch

refactor(plot_one): remove extra spaces after minus sign

c2bab55

Kully changed the title ~~Icicle Trace - DRAFT~~ Icicle Trace Apr 5, 2021

Coding-with-Adam added 2 commits April 5, 2021 16:03

fix(icicle): if you click icicle, the root color does not expand acro…

4402d45

…ss the trace domain anymore

refactor(plot_one): add semicolon

dd3cc91

Kully requested review from alexcjohnson and nicolaskruchten April 5, 2021 22:16

archmoj marked this pull request as ready for review May 12, 2021 18:31

merge with master branch

3b17a90

update icicle_values_colorscale baseline

fdff0e7

archmoj reviewed Jun 11, 2021

View reviewed changes

src/traces/icicle/partition.js Outdated Show resolved Hide resolved

archmoj added 3 commits June 11, 2021 14:45

reuse treemap draw in icicle

eaad10d

reuse flip tree in icicle

d96f3f1

avoid multiple string comparisons

c57dda5

nicolaskruchten reviewed Jun 15, 2021

View reviewed changes

archmoj added this to the NEXT milestone Jun 15, 2021

archmoj added the status: has TODOs label Jun 16, 2021

Coding-with-Adam and others added 3 commits June 16, 2021 17:03

docs(icicle): clarify the description; set default to 0

7140c3e

update baselines with zero pad

ae4be8c

add pad to uniformtext_icicle mock

7de2bd9

archmoj merged commit 40f058c into master Jun 17, 2021

archmoj deleted the icicle-trace-base branch June 17, 2021 17:24

This was referenced Jun 17, 2021

update flaky list of image-test #5737

Merged

Add missing noCI tests required to add the new trace type icicle #5745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Icicle Trace #5546

Icicle Trace #5546

Kully commented Mar 11, 2021 •

edited

Loading

PHP-Proxy

Kully commented Mar 11, 2021 •

edited

Loading

nicolaskruchten commented Mar 11, 2021

Kully commented Mar 12, 2021

archmoj commented Mar 16, 2021

Kully commented Mar 16, 2021 •

edited

Loading

archmoj commented May 12, 2021

Kully commented May 19, 2021

archmoj commented May 19, 2021 •

edited

Loading

Kully commented May 31, 2021

archmoj commented Jun 11, 2021

nicolaskruchten Jun 15, 2021

nicolaskruchten Jun 15, 2021

Kully Jun 16, 2021

nicolaskruchten Jun 15, 2021

Kully Jun 16, 2021

archmoj commented Jun 17, 2021


		flip: treemapAttrs.tiling.flip,

		pad: treemapAttrs.tiling.pad,

Icicle Trace #5546

Icicle Trace #5546

Conversation

Kully commented Mar 11, 2021 • edited Loading

Setting up the PR

List of Tasks

PHP-Proxy

Kully commented Mar 11, 2021 • edited Loading

nicolaskruchten commented Mar 11, 2021

Kully commented Mar 12, 2021

archmoj commented Mar 16, 2021

Kully commented Mar 16, 2021 • edited Loading

archmoj commented May 12, 2021

Kully commented May 19, 2021

archmoj commented May 19, 2021 • edited Loading

Kully commented May 31, 2021

archmoj commented Jun 11, 2021

nicolaskruchten Jun 15, 2021

Choose a reason for hiding this comment

nicolaskruchten Jun 15, 2021

Choose a reason for hiding this comment

Kully Jun 16, 2021

Choose a reason for hiding this comment

nicolaskruchten Jun 15, 2021

Choose a reason for hiding this comment

Kully Jun 16, 2021

Choose a reason for hiding this comment

archmoj commented Jun 17, 2021

Kully commented Mar 11, 2021 •

edited

Loading

Kully commented Mar 11, 2021 •

edited

Loading

Kully commented Mar 16, 2021 •

edited

Loading

archmoj commented May 19, 2021 •

edited

Loading