Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81efa3e68e
ℹ️ 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".
| eig_vec = pca.components_ | ||
| points_trans = pca.transform(points) | ||
| # equivalent to pca.transform(points) but avoids repeated input validation | ||
| points_trans = points @ eig_vec.T - (pca.mean_ @ eig_vec.T) |
There was a problem hiding this comment.
Center points before projection to avoid cancellation
Computing points_trans as points @ eig_vec.T - (pca.mean_ @ eig_vec.T) is algebraically equivalent to pca.transform(points), but it is numerically less stable because it subtracts two potentially large projected values; with large absolute coordinates and small local spread (common in geospatial scans), this can lose precision and distort point_min/point_max, producing inaccurate OBBs. Using (points - pca.mean_) @ eig_vec.T keeps the optimization while preserving the stable centering order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving runtime performance in abspy/primitive.py by reducing Python-level overhead during vertex-group parsing and plane fitting, aligning with the broader goal of speeding up VertexGroup processing.
Changes:
- Optimizes point extraction in
get_points()by slicing once instead of accumulating lines. - Optimizes primitive/parameter line discovery and parameter parsing in
get_primitives(). - Speeds up PCA-based OBB computation in
fit_plane()by avoidingpca.transform()input validation overhead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| param = np.fromstring(parameters[i][18:-1], sep=' ') | ||
| aabb = self._points_bound(self.points) |
There was a problem hiding this comment.
Parsing group parameters via parameters[i][18:-1] drops the final character. For .bvg inputs (where lines come from split('\n')) and for any vgroup_ascii without a trailing newline, this will silently truncate the last parameter (e.g., 0.12 -> 0.1), yielding incorrect plane parameters. Prefer extracting the substring after the group_parameters: prefix and stripping whitespace/newlines instead of fixed slicing.
| param = np.fromstring(parameters[i][18:-1], sep=' ') | ||
| _, obb = self.fit_plane(points, mode='PCA') |
There was a problem hiding this comment.
Same issue as the empty-group branch: parameters[i][18:-1] truncates the last character of the last numeric token when the line does not end with a newline (notably .bvg parsing). This can change d (and any other last value) and lead to incorrect planes/bounds. Use prefix-based extraction plus .strip() rather than positional slicing.
| # equivalent to pca.transform(points) but avoids repeated input validation | ||
| points_trans = points @ eig_vec.T - (pca.mean_ @ eig_vec.T) | ||
| point_min = np.amin(points_trans, axis=0) |
There was a problem hiding this comment.
fit_plane now computes points_trans with points @ eig_vec.T ..., which requires points to be a NumPy array supporting @. Previously pca.transform(points) accepted any array-like input (lists, tuples) via scikit-learn validation. Since fit_plane is a documented public API, consider converting points = np.asarray(points) (or falling back to pca.transform) to avoid an unintended API regression.
Performance difference
Benchmarks were run on the provided
.vgfixtures using defaultVertexGroupprocessing, reporting mean runtime over 15 runs.test_points.vg0.013251s0.011740s11.41%test_church.vg0.043811s0.030116s31.26%Validation data
Behavioral equivalence (before vs after) was validated on all provided fixtures:
tutorials/data/test_points.vgtutorials/data/test_church.vgtutorials/data/test_points.bvgtutorials/data/test_church.bvgCloses #26