-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sunburst/treemap path #2006
Sunburst/treemap path #2006
Conversation
@@ -82,6 +82,7 @@ | |||
colref_desc, | |||
"Values from this column or array_like are used to set ids of sectors", | |||
], | |||
path=[colref_type, colref_desc], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops yes. The PR is not completely ready but I'll still appreciate your thoughts about the API and how it's implemented, then I will polish if this is the right direction, unless there is some refactoring to do first.
Thank you @emmanuelle for working on this! This will make is way easier for everyone to produce these beautiful plots, and I will certainly be a user of this. I have a few remarks/questions:
|
Thanks for taking a look @mwouts ! Re intermediate nodes, I think we should try to accept null/None/NaN for trailing path elements somehow, yes. Re values for intermediate nodes, let's see how far we can get by having users provide them with the approach above. We could accept an aggregating function as a parameter eventually if this is too limiting :) Also, I don't think we should add an epsilon, as this will be released as part of 4.5.0, which will include the fix for plotly/plotly.js#4405 |
Thanks for taking a look @mwouts ! @nicolaskruchten you were mentioning that we could accept a dictionary of lambda functions for doing the aggregation, is this what you had in mind? |
In order to address the case of lines with None, maybe we should have |
@mwouts we were also looking for a way to address the case of https://plot.ly/python/sunburst-charts/#sunburst-chart-with-a-continuous-colorscale but it seems hard to use a generic API for this... |
After all I included an EPS fix because otherwise my doc example (with |
I wrote the doc for sunburst but not for treemap since it will be mostly a copy so I'll update after a round of review. |
Thanks! Glad that helps!
Well don't you think the same approach, generalized to magic underscore notation, could work? In my example (scroll to the second treemap plot) I have defined |
Some feedback to start with:
|
Overall this is really, really awesome though, I love the symmetry with |
PS: I think we can probably use a similar API for |
As a motivating example for the implicit behaviour in continuous color, think about a case like With my scheme, the |
Question about color: if |
Yep the current behaviour is fine when color is not passed IMO |
for i, level in enumerate(path): | ||
df_tree = pd.DataFrame(columns=df_all_trees.columns) | ||
if not agg_f: | ||
dfg = df.groupby(path[i:]).sum(numerical_only=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under what circumstances do we do this, and what's the reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's if neither values
nor color
was passed. I think I need to use some sort of aggregation function because after I call reset_index
to get the groups, but maybe it's possible to do it in a more elegant/efficient way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could get rid of this part with the count_colname
logic.
This causes an error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have addressed all your comments!
Still WIP, TODO