-
-
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
Correct bug when color and values correspond to the same color in treemap or sunburst #2591
Conversation
Note: this also fixes a new problem introduced in #2544 |
packages/python/plotly/plotly/tests/test_core/test_px/test_px_functions.py
Outdated
Show resolved
Hide resolved
packages/python/plotly/plotly/tests/test_core/test_px/test_px_functions.py
Show resolved
Hide resolved
aggfunc_color = "sum" | ||
new_value_col_name = args["values"] + "_total" | ||
df[new_value_col_name] = df[args["values"]] | ||
args["values"] = new_value_col_name |
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.
so like this, the <col>_total
appears in the hoverlabel as the value. This is what we want? Not sure what would be better...
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.
Maybe we want <col>_sum
instead? or <col>_weighted_average
for the other one? not sure
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.
<col>_color
for the other one maybe?
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'm fine changing total
to sum
. I put total
like branchvalues='total'
. We could also put a suffix for the colors
column but if we want to minimize the occurrence of such suffixes, we'd better put it on the values
column (only in the case where it's the same as the colors
column).
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.
ok let's do sum
and merge this thing :)
💃 when changed to Oh and a changelog entry please? |
will merge after the CI runs |
Closes #2567