feat(validation): add intrinsic orientability witnesses - #526
Conversation
- Expose orientation certificates for pure 2D and 3D simplicial complexes. - Report typed non-orientability failures during Level 3 PL-manifold validation. - Document intrinsic orientation and refresh the generated validation diagrams. Closes #520
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a public ChangesIntrinsic PL orientability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Triangulation
participant OrientationWitness
participant Tds
participant Level3Validation
Triangulation->>OrientationWitness: compute simplex reversal assignments
OrientationWitness->>Tds: inspect ordinary shared-facet parity
Tds-->>OrientationWitness: return neighboring constraints
OrientationWitness-->>Triangulation: return witness or NonOrientable
Level3Validation->>Triangulation: validate intrinsic orientability
Triangulation-->>Level3Validation: propagate validation result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 92.45% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 92.45% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (de8b861) 80257 73129 91.12% Head commit (5758d78) 80533 (+276) 73384 (+255) 91.12% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#526) 278 257 92.45% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
api
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/validation.rs (2)
318-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
NonOrientablecarries less diagnostic detail than sibling error variants.The intrinsic simplex-orientation constraints contain a parity obstruction.
#[error(
"Intrinsic complex is non-orientable: the shared facet between simplices {simplex1_key:?} and {simplex2_key:?} closes with contradictory orientation parity"
)]
NonOrientable {
/// First simplex on the contradictory adjacency.
simplex1_key: SimplexKey,
/// Second simplex on the contradictory adjacency.
simplex2_key: SimplexKey,
}, Other variants in the same enum (e.g.BoundaryFacetInClosedTopology,IsolatedVertex) include both the key and UUID plus facet index for stable, human-readable identification.NonOrientableonly has bareSimplexKeys (printed via{:?}as opaque slotmap debug output) and omits the facet index that caused the contradiction, even thoughfacet_index/mirror_indexare already computed locally at the call site (lines 1375-1390). This makes the error harder to act on than its siblings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/validation.rs` around lines 318 - 328, Expand the NonOrientable error variant to carry the UUIDs and the shared facet index alongside the simplex keys, matching the diagnostic detail of BoundaryFacetInClosedTopology and IsolatedVertex. Update its error message and the call site using facet_index/mirror_index so both contradictory simplices and the triggering facet are reported with stable, human-readable identifiers.
385-450: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a hash-map-backed
OrientationWitnessfor O(1)is_reversedlookups.The internal computation already builds
FastHashMap<SimplexKey, bool>during BFS (line 1315), but the final struct pub struct OrientationWitness {
assignments: Vec<(SimplexKey, bool)> converts to aVec, andis_reverseddoes a linear scan: pub fn is_reversed(&self, simplex_key: SimplexKey) -> Option {
self.assignments
.iter()
.find_map(|&(key, reversed)| (key == simplex_key).then_some(reversed))
}. Callers who look up reversal state per-simplex (the pattern shown in the doctest itself) get O(N) per lookup instead of O(1), turning natural usage patterns into O(N²). SinceSimplexKeyalready supports hashing, storing aFastHashMap<SimplexKey, bool>internally would preserve the opaque API while fixing this.Also applies to: 1399-1409
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/validation.rs` around lines 385 - 450, Change OrientationWitness to store the existing FastHashMap<SimplexKey, bool> assignments instead of a Vec, and update its constructor/conversion path from the BFS result to retain that map. Update len, is_empty, is_reversed, and iter to use the map while preserving the current public behavior and iterator item type, so reversal lookups are O(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/validation.rs`:
- Around line 318-328: Expand the NonOrientable error variant to carry the UUIDs
and the shared facet index alongside the simplex keys, matching the diagnostic
detail of BoundaryFacetInClosedTopology and IsolatedVertex. Update its error
message and the call site using facet_index/mirror_index so both contradictory
simplices and the triggering facet are reported with stable, human-readable
identifiers.
- Around line 385-450: Change OrientationWitness to store the existing
FastHashMap<SimplexKey, bool> assignments instead of a Vec, and update its
constructor/conversion path from the BFS result to retain that map. Update len,
is_empty, is_reversed, and iter to use the map while preserving the current
public behavior and iterator item type, so reversal lookups are O(1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 062db724-ec0a-45ef-83e4-3c53034b4cb5
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockdocs/assets/validation/validation_hierarchy.pngis excluded by!**/*.pngdocs/assets/validation/validation_level_3_intrinsic_pl_topology.pngis excluded by!**/*.png
📒 Files selected for processing (10)
Cargo.tomlREADME.mddocs/ORIENTATION_SPEC.mddocs/validation.mdnotebooks/01_validation.ipynbsrc/core/algorithms/incremental_insertion.rssrc/core/tds/errors.rssrc/core/tds/validation.rssrc/core/validation.rssrc/lib.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
========================================
Coverage 91.09% 91.10%
========================================
Files 88 88
Lines 80046 80322 +276
========================================
+ Hits 72920 73175 +255
- Misses 7126 7147 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Preserve BFS assignments for constant-time simplex reversal lookups. - Report stable simplex UUIDs and shared-facet indices for parity obstructions. - Defer periodic quotient orientability to its dedicated validation contract. - Refresh the generated validation paper artifact.
coderabbitai
Bot
removed
the
documentation
1 participant
Closes #520