Skip to content
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

fixing hover_data and hover_name bugs in path API #2524

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

nicolaskruchten
Copy link
Contributor

This approach is a bit less brittle I think.

assert fig.layout.coloraxis.colorbar.title.text == "lifeExp"

df = px.data.tips()
fig = px.sunburst(df, path=["sex", "day", "time", "smoker"], hover_name="smoker")
Copy link
Contributor Author

@nicolaskruchten nicolaskruchten Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually errors out on master (because smoker is in path but isn't accounted for in hover_name)

fig = px.sunburst(
df, path=["continent", "country"], color="lifeExp", hover_data=df.columns
)
assert fig.layout.coloraxis.colorbar.title.text == "lifeExp"
Copy link
Contributor Author

@nicolaskruchten nicolaskruchten Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on master this appears as lifeExpadditional_col_for_hover because we rewrite args["color"]

assert "%{hovertext}" in fig.data[0].hovertemplate # represented as '%{hovertext}'

df = px.data.tips()
fig = px.sunburst(df, path=["sex", "day", "time", "smoker"], custom_data=["smoker"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also errors out on master (because smoker is in path but isn't accounted for in custom_data)

for col_name in path:
series_to_copy = df[col_name]
new_col_name = col_name + "_path_copy"
path = [new_col_name if x == col_name else x for x in path]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new approach is just to make a full copy of everything in path even if they're not in color or whatever and not touch any of the other args, instead of spot-checking color and hover_data... Maybe there's a case here I'm missing but it's not in the tests and this approach seems more future-friendly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we don't need the if x == col_name here? Since x is in path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an elegant solution. I had not thought that column names in path were disposable since they would not appear in the final hovertemplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right that comprehension is weird... check out the new version?

@emmanuelle
Copy link
Contributor

Thanks for the improvement! I left a small comment since I think one if check can be removed but otherwise it's good to merge! 💃

@nicolaskruchten nicolaskruchten merged commit c5f2860 into master Jun 5, 2020
@nicolaskruchten nicolaskruchten added this to the 4.8.2 milestone Jun 5, 2020
@nicolaskruchten nicolaskruchten deleted the path_hover_fixes branch June 19, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants