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

[bugfix] Use binarySearch for larger and smaller in HistogramCombiner #5895

Merged
merged 1 commit into from Apr 25, 2019

Conversation

@chrisvittal
Copy link
Collaborator

@chrisvittal chrisvittal commented Apr 17, 2019

Fixes #5846

@chrisvittal
Copy link
Collaborator Author

@chrisvittal chrisvittal commented Apr 17, 2019

I will add a test.

Loading

Copy link
Collaborator

@tpoterba tpoterba left a comment

waiting on test (req changes to appease scorecard)

Loading

Copy link
Collaborator

@tpoterba tpoterba left a comment

see the comments on #5846 -- I now think that -0.0 should go in the smaller category. We can do this by checking for nans first, then using java.lang.Double.compare in the rest of the comparisons.

Loading

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Apr 18, 2019

also d.isNan does an implicit conversion to java.lang.Double

🤦‍♂️

Loading

@chrisvittal chrisvittal force-pushed the issue-5846 branch 2 times, most recently from 58ebdc7 to 3c6d2d9 Apr 25, 2019
@chrisvittal chrisvittal dismissed tpoterba’s stale review Apr 25, 2019

Okay this should be way better now.

@chrisvittal chrisvittal changed the title Coerce -0 to +0 in HistogramCombiner Use binarySearch for larger and smaller in HistogramCombiner Apr 25, 2019
@chrisvittal chrisvittal changed the title Use binarySearch for larger and smaller in HistogramCombiner [bugfix] Use binarySearch for larger and smaller in HistogramCombiner Apr 25, 2019
@danking danking merged commit 8d1f65e into hail-is:master Apr 25, 2019
1 check passed
Loading
@chrisvittal chrisvittal deleted the issue-5846 branch May 9, 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
Linked issues

Successfully merging this pull request may close these issues.

3 participants