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

Compute histogram bin positions beyond length 5000 #1887

Merged
merged 2 commits into from
Jul 19, 2017

Conversation

etpinard
Copy link
Contributor

Fixes #1682

@alexcjohnson does this look acceptable?

- to correctly handle (very) small bins specs
- to avoid, infinite loops, break in while loop when
  tick increment does not converge.
@etpinard etpinard added status: reviewable bug something broken labels Jul 17, 2017
@alexcjohnson
Copy link
Collaborator

Makes sense, the fix for #484 is going to be very similar. But, like my comment there #484 (comment) I wonder if we still want to have some hard-coded upper bound on the total number, even if it's super high like 1e6?

@etpinard
Copy link
Contributor Author

though I do think we need some sort of upper bound to prevent locking the browser - this can happen easily for example if you specify an explicit tick spacing and then zoom out a lot.

Yeah, a super high upper bound would make sense. Maybe we could put that upper bound somewhere in a constants file and reuse it in all our while loops? Though, locking the browser is less concerning here as this while loop is called in the calc step as opposed to #484 which is called in the plot step.

@etpinard
Copy link
Contributor Author

wonder if we still want to have some hard-coded upper bound on the total number, even if it's super high like 1e6?

done in eef9158

@alexcjohnson
Copy link
Collaborator

💃

@etpinard etpinard merged commit 35c84fa into master Jul 19, 2017
@etpinard etpinard deleted the histogram-mucho-bins-fix branch July 19, 2017 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants