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

♻️ check for all same groups #3767

Merged

Conversation

jvdd
Copy link
Contributor

@jvdd jvdd commented Jun 9, 2022

Check for all same group instead of checking for all one group (see #3765)

@nicolaskruchten
Copy link
Contributor

Unfortunately, this is not a backwards-compatible change... For example, the following:

import plotly.express as px
fig = px.scatter(x=[1,2,3], y=[1,2,3], color=["a","a","a"])
fig.show()

On master this yields

newplot (1)

but on this branch the legend has disappeared and the tooltips no longer include "color=a" etc.

@nicolaskruchten
Copy link
Contributor

(note: we have "Percy" image tests that compare the graphical output of PRs to that of master, but for some reason these don't run on pull requests from folks outside the Plotly organization. The example I gave above would likely cause this test to fail... the first commit on my PR failed this test as well for similar reasons)

@nicolaskruchten
Copy link
Contributor

Note: #3761 does appear backwards-compatible because it independently computes sorted_group_names

@jvdd
Copy link
Contributor Author

jvdd commented Jun 9, 2022

Oh, I see! I believe it is because of the hard-coding of the orders being {} and the sorted_group_names being [("",....)].
I'll update the logic of constructing orders and sorted_group_names to match the current master behavior

@jvdd
Copy link
Contributor Author

jvdd commented Jun 9, 2022

Note: #3761 does appear backwards-compatible because it independently computes sorted_group_names

I guess we both identified the issue at the same time! 😄

@nicolaskruchten
Copy link
Contributor

OK, this looks great, thanks! I have an idea about how to reuse the existing uniques() call to compute all_same_group more efficiently, but I'll do that on my branch.

Thanks for pushing through this with me :)

@nicolaskruchten nicolaskruchten merged commit dd2137b into plotly:one_group_short_circuit Jun 9, 2022
@jvdd
Copy link
Contributor Author

jvdd commented Jun 9, 2022

Great! And thank you as well @nicolaskruchten for maintaining this amazing library & the quick responses! :)

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