Randomized closest 2 points algorithm#6251
Randomized closest 2 points algorithm#6251bri-harris wants to merge 23 commits intoTheAlgorithms:masterfrom
Conversation
bri-harris
requested review from
BamaCharanChhandogi,
DenizAltunkapan,
alxkm,
siriak,
vil02 and
yanglbme
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6251 +/- ##
============================================
+ Coverage 73.98% 74.09% +0.10%
- Complexity 5346 5364 +18
============================================
Files 677 678 +1
Lines 18652 18697 +45
Branches 3621 3631 +10
============================================
+ Hits 13800 13853 +53
+ Misses 4294 4283 -11
- Partials 558 561 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@DenizAltunkapan could you take a look at infer? |
Of course @siriak . I'll take a look, when i find time |
|
The current build of facebook/infer is failing (see), which also causes our GitHub Actions workflow to fail. To temporarily stabilize our CI pipeline, I used the last known working commit. Once the upstream build is fixed, I will update our workflow to use the latest version again. I will open a new Pullrequest, when it's working again. 😄 @siriak @bri-harris |
DenizAltunkapan
left a comment
DenizAltunkapan
left a comment
There was a problem hiding this comment.
Could you please implement the requested changes? @bri-harris Thank you!
There was a problem hiding this comment.
Could you please check the requested changes and fix the failing pipeline? Thank you! @bri-harris
| assertNotNull(closestPair[0]); | ||
| assertNotNull(closestPair[1]); | ||
|
|
||
| assertTrue((closestPair[0].equals(points.get(0)) && closestPair[1].equals(points.get(1))) || (closestPair[1].equals(points.get(0)) && closestPair[0].equals(points.get(1)))); |
There was a problem hiding this comment.
Are you sure this test is correct? Because the .equals method is not overwritten in the Point class
| assertNotNull(closestPair[0]); | ||
| assertNotNull(closestPair[1]); | ||
|
|
||
| assertTrue((closestPair[0].equals(points.get(0)) && closestPair[1].equals(points.get(1))) || (closestPair[1].equals(points.get(0)) && closestPair[0].equals(points.get(1)))); |
There was a problem hiding this comment.
could you please refine and improve the test a bit? The likely cause of the NullPointer warning is that the analysis tool doesn't recognize assertNotNull() as a safe null check—assigning the value to a variable before asserting might help. Thats probably why infer is failing
| } | ||
| } | ||
| } | ||
| grid.put(key, p); |
There was a problem hiding this comment.
I think currently, only one point is stored per grid cell, overwriting any existing points. This can cause missed closest pairs when multiple points fall into the same cell. It is recommended to store a list of points per cell to ensure correctness...
|
@bri-harris still working on it? |
4 participants
clang-format -i --style=file path/to/your/file.java