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

Fix selection of histograms with multiple traces #2771

Merged
merged 3 commits into from
Apr 23, 2021

Conversation

meffmadd
Copy link
Contributor

@meffmadd meffmadd commented Sep 15, 2020

This change addresses the selection behaviour of histograms with more than one trace and fixes a bug introduced in my last change #2711.

In the previous change, the point_indexes were sorted before being returned. This is not a problem when there is only one trace, however, with multiple traces, the indices of the values get mixed and result in incorrect behaviour.

So for example consider two traces with a random distribution and both traces are being selected. Firstly, all the indices are combined into one large array with the corresponding trace_indices [0,0,…,0,0,1,1 …,1,1].
Then the index-array is sorted and all the indices get reordered while the trace information is unchanged, which causes indices being assigned to the incorrect traces later on.

A quick solution is not sorting the array, which results in correct behaviour. This has the effect that the results are also not sorted in Python in each trace. Please let me know if this is an issue.

Screenshot 2020-09-15 at 12 57 24

@meffmadd
Copy link
Contributor Author

Are there any updates on this issue, @nicolaskruchten, @alexcjohnson and/or @jonmmease?

In the meantime I also implemented sorting for each trace individually, however, this comes with a performance penalty so let me know if this is necessary for multiple traces.

@nicolaskruchten
Copy link
Contributor

@jonmmease if you have a sec to look at this one I'd appreciate it :)

@jonmmease
Copy link
Contributor

Not sorting the indices is fine. This looks good to me. Thanks, @meffmadd!

@meffmadd
Copy link
Contributor Author

meffmadd commented Dec 7, 2020

Thank you for reviewing the changes! I never pushed the version where the indices are sorted so the PR could be merged.

@meffmadd
Copy link
Contributor Author

Hey @nicolaskruchten, I think this PR fell through the cracks but as far as I understand it, it was approved and could be merged immediately without causing problems. It would really help my project to have this functionality. Thanks!

@nicolaskruchten
Copy link
Contributor

Indeed, we've not done a good job of pushing this PR along, and I apologize. The next version of Plotly.py will be v5.0 and I expect it will come out in about a month, and I'll try to get this merged and QA'ed for that release.

@meffmadd
Copy link
Contributor Author

Thank you very much and no problem! I was busy either way the last couple of months but recently found the time to work on my stuff again.

@jonmmease
Copy link
Contributor

Finally merging this, now that we're pulling version 5 changes into master. Thanks again @meffmadd!

@meffmadd
Copy link
Contributor Author

Thanks, always happy to contribute!

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