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

adding pie, treemap, sunburst, funnel and funnelarea to px #1909

Merged
merged 15 commits into from
Nov 28, 2019

Conversation

emmanuelle
Copy link
Contributor

@emmanuelle emmanuelle commented Nov 18, 2019

No description provided.

@emmanuelle emmanuelle changed the title [WIP] first draft of adding pie, sunburst, funnel and funnelarea adding pie, sunburst, funnel and funnelarea to px Nov 19, 2019
@nicolaskruchten
Copy link
Contributor

This is all pretty good, modulo inline comments.

I'd really like to have support for color in pie/funnel/treemap/sunburst the day we release this though... I feel like adding continuous-color support for treemap/sunburst should be pretty trivial (i.e. it should "just work") and then discrete-color support will be harder because for these 4 traces it doesn't work at all the way it does in the other traces: it's not "groupable". But we should try to do it anyway :)

@nicolaskruchten
Copy link
Contributor

I'd really like to have support for color in pie/funnel/treemap/sunburst the day we release this

This would basically be the "value added" of PX for these traces IMO, as compared to using the graph objects directly, so that's why I think it's important :)

@nicolaskruchten
Copy link
Contributor

The way I would implement discrete color is basically in make_trace_kwargs with another gross type-specific exception inside the elif k == "color": block, which would just hardcode the marker colors.

@nicolaskruchten
Copy link
Contributor

Extra requirement: if color_discrete_sequence is passed into pie/funnelarea/treemap/sunburst, we should probably assign it to layout.<whatever>colorway, so that something like px.pie(values=[1,2,3], color_discrete_sequence=["red", "green", "blue"]) will do the expected thing.

@emmanuelle
Copy link
Contributor Author

Hum, interesting to see that the test fail because the color of the funnel is not the same.

@emmanuelle
Copy link
Contributor Author

Do we want to accept color_discrete_sequence='Viridis'? At the moment it only works with a px object.

@nicolaskruchten
Copy link
Contributor

it's normal that color_discrete_sequence only works with an array of colors, yes. It's not clear what we would do if we got Viridis, as that scale doesn't have a fixed number of categorical colors...?

@emmanuelle
Copy link
Contributor Author

ok good, so nothing more to do for this :-).

@nicolaskruchten
Copy link
Contributor

@emmanuelle some tweaks in that last commit, LMK what you think.

@nicolaskruchten nicolaskruchten added this to the v4.3.1 milestone Nov 28, 2019
mapping[cat] = args["color_discrete_sequence"][
len(mapping) % len(args["color_discrete_sequence"])
]
result["marker"]["colors"].append(mapping[cat])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting, I did not know that marker_colors could be a dictionary, I thought it had to be a list/array.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it does have to be a list, and in this case it is...?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same code you had, just inlined :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes I had read too fast!

@emmanuelle
Copy link
Contributor Author

emmanuelle commented Nov 28, 2019

Thanks, looks good to me. Should we give each other a 💃 ? :-)

@nicolaskruchten nicolaskruchten merged commit 3e35a6b into master Nov 28, 2019
@emmanuelle emmanuelle deleted the px-pie-etc branch December 2, 2019 17:03
@nicolaskruchten nicolaskruchten changed the title adding pie, sunburst, funnel and funnelarea to px adding pie, treemap, sunburst, funnel and funnelarea to px Dec 9, 2019
@jonmmease jonmmease modified the milestones: v4.3.1, v4.4.0 Dec 10, 2019
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.

None yet

3 participants