Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af83cc2118
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for c_index in range(1, len(points)): | ||
| if not is_colinear(a, points[b_index], points[c_index], tol): | ||
| break |
There was a problem hiding this comment.
When the first three points are collinear and the next candidate is only just outside tol, this loop stops at that candidate, so the cross-product normal can itself be about tol; is_perpendicular_vector_vector then compares the unnormalised dot product to the same absolute tolerance and accepts points far from the plane. With the default tolerance, [[0,0,0],[1,0,0],[2,0,0],[10,0,0],[0,2.2e-10,0],[0,1,.4],[0,.5,-.4]] now returns True (both final points are 0.4 away from the selected z=0 plane, and the parent returned False). Choose c by maximum area/distance from the baseline, or otherwise condition the normal, before applying the plane checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a915f4b. The fallback now chooses the maximum-area third point. The four tolerance/order regressions, 1,032-test suite and documentation build pass. All 18,144 original image results remain unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a915f4ba78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| b_index = max(range(1, len(points)), key=lambda i: distance_point_point(a, points[i])) | ||
| c_index = max(range(1, len(points)), key=lambda i: area_triangle([a, points[b_index], points[i]])) |
There was a problem hiding this comment.
When the first three points are collinear, this only maximizes triangles that contain fixed a and farthest b, so it can still select a weaker normal than a triangle elsewhere in the same cloud. The new evidence after the earlier third-point fix is [[0,0,0],[.05,0,0],[.1,0,0],[0,1e-4,0],[0,-1e-4,0],[.1,0,0],[0,0,9e-5]]: with the default tolerance this revision returns True whereas the parent returns False; the selected normal gives the last point a dot product of 9e-10, but the plane from points 3–5 gives 1.8e-9, and the point is actually 9e-5 away. Search all candidate triangles (or normalize the plane normal before testing distances) so the fallback cannot accept this noncoplanar cloud.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f76a38e. The predicate now divides by the normal length and uses distance tolerance throughout, including collinear detection. Your example is rejected; all 1,045 tests pass, and all 18,144 original image results remain unchanged.
antonsoo
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f76a38ebe0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break | ||
| if not temp: | ||
| return True | ||
| c_index = max(range(1, len(points)), key=lambda i: distance_point_line(points[i], (a, b))) |
There was a problem hiding this comment.
When samples lie on both sides of an underlying plane near tol, choosing three samples and forcing them to have zero residual can reject a cloud whose every point is within the documented distance tolerance. For [[0,0,0],[1,0,0],[0,1,0],[10,10,6e-10],[10,-10,-6e-10],[-10,10,-6e-10],[-10,-10,6e-10]], every point is at most 6e-10 from z=0 and the parent accepts it, but this revision selects indices 3 and 4 for the basis and measures the opposite corners at 1.2e-9, returning False with the default 1e-9 tolerance. Use a fitted/consensus plane, or otherwise allow residuals for the basis points, so an absolute point-to-plane tolerance does not produce these false negatives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5d3919. The plane offset now allows residuals for every point; if the initial plane fails, the existing pure-Python fitter runs in a stable local frame. Your example and all 96 noisy-plane configurations pass. All 1,069 tests pass, and every earlier recorded-workload result stays unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5d3919998
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for point, distance in zip(points, distances): | ||
| vector = subtract_vectors(point, a) | ||
| local.append([dot_vectors(vector, u), dot_vectors(vector, v), distance]) | ||
| _, normal = bestfit_plane(local) |
There was a problem hiding this comment.
When a noisy cloud has duplicate or densely sampled locations, bestfit_plane weights those samples and does not minimize the maximum residual, so this fallback can reject a cloud even though one plane satisfies the documented tolerance. For example, at the default tolerance, [[0,0,0],[1,0,0],[0,1,0]] + 5 * [[1,2,9e-10],[1,-2,9e-10],[-1,2,-9e-10],[-1,-2,-9e-10]] + [[10,0,-9e-10]] returns False: the fallback's centered half-width is about 1.27e-9, although every input is at most 9e-10 from z=0 (and the parent implementation accepts it). Use a minimax/consensus plane, or otherwise do not treat failure of this sample-weighted fit as proof of non-coplanarity.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
is_coplanardiscards complete collinear triples while searching for a plane, then checks only the remaining points. Discarded points can lie outside that plane, so nonplanar clouds can returnTrue.Polygon.is_planarinherits the result. For example:Retain every input and use a stable baseline to define an initial plane. Center that plane between the extreme projections, allowing residuals on its basis points as well as the other samples. If it exceeds tolerance, fit a plane to all points in a stable local frame using the existing pure-Python fitter, then repeat the centered distance check. This handles noisy coplanar data without making three noisy samples exact constraints.
tolconsistently measures perpendicular distance; signature and dependencies stay the same.Regressions cover discarded triples, duplicate prefixes, point order, weak normals, coordinate scales and noisy planes. All 1,069 tests pass, with 12 skips, in the
compas3Conda environment on Python 3.12. Independent recorded-image audits retain all prior corrections and exact controls across the original and smaller-unit clouds. A constructed noisy-plane audit derived from recorded scan coordinates corrects 72 rejected configurations out of 96, with an independent known-plane distance reference. These are repeated configurations of the source assets.What type of change is this?
Checklist
CHANGELOG.mdunderUnreleased.invoke test).invoke lint).invoke formatleaves all 472 files unchanged. Changed files pass Ruff lint and format checks. Full read-only lint reports the same two existing Rhino import-order errors on base and head; the automatic unrelated edits frominvoke lintwere restored.The Sphinx HTML build passes. Documentation warnings and the unchanged baseline lint diagnostics are recorded with the validation results.