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

@meffmadd meffmadd commented Sep 25, 2020

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
Member

@nicolaskruchten nicolaskruchten commented Nov 19, 2020

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

@jonmmease
Copy link
Member

@jonmmease jonmmease commented Nov 19, 2020

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

@meffmadd
Copy link
Contributor Author

@meffmadd 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

@meffmadd meffmadd commented Mar 14, 2021

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
Member

@nicolaskruchten nicolaskruchten commented Mar 19, 2021

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

@meffmadd meffmadd commented Mar 20, 2021

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.

@nicolaskruchten nicolaskruchten added this to To sort in v5 roadmap Apr 21, 2021
@nicolaskruchten nicolaskruchten moved this from To sort to PRs in v5 roadmap Apr 22, 2021
@jonmmease
Copy link
Member

@jonmmease jonmmease commented Apr 23, 2021

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

@jonmmease jonmmease merged commit b343fcc into plotly:master Apr 23, 2021
14 of 15 checks passed
14 of 15 checks passed
ci/circleci: build-doc Your tests failed on CircleCI
Details
ci/circleci: check-code-formatting Your tests passed on CircleCI!
Details
ci/circleci: plotlyjs_dev_build Your tests passed on CircleCI!
Details
ci/circleci: python-2-7-orca Your tests passed on CircleCI!
Details
ci/circleci: python-2.7-core Your tests passed on CircleCI!
Details
ci/circleci: python-2.7-optional Your tests passed on CircleCI!
Details
ci/circleci: python-3-7-orca Your tests passed on CircleCI!
Details
ci/circleci: python-3.5-core Your tests passed on CircleCI!
Details
ci/circleci: python-3.5-optional Your tests passed on CircleCI!
Details
ci/circleci: python-3.6-core Your tests passed on CircleCI!
Details
ci/circleci: python-3.6-optional Your tests passed on CircleCI!
Details
ci/circleci: python-3.7-core Your tests passed on CircleCI!
Details
ci/circleci: python-3.7-optional Your tests passed on CircleCI!
Details
ci/circleci: python-3.7-percy Your tests passed on CircleCI!
Details
ci/circleci: python-3.7-plot_ly Your tests passed on CircleCI!
Details
v5 roadmap automation moved this from PRs to Closed Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants