Add TriFold semiring reasoning head

[research-developer]

Summary

• implement a TriFold semiring along with fold/unfold reasoning operators for semantic triples
• integrate the TriFold reasoner into the hierarchical layer to expose recursive subject–predicate–object coherence signals
• document the new capability and cover it with focused unit tests

Testing

• Not run (not requested)

---

https://chatgpt.com/codex/tasks/task_e_68f70bf0f9708324a148b7edf6c00a9e

[claude]

Pull Request Review: Add TriFold Semiring Reasoning Head

This PR introduces a TriFold semiring with fold/unfold operators. While technically sophisticated, there are significant architectural and design concerns.

Critical Issues

1. Architectural Misalignment with Project Roadmap
According to CLAUDE.md, this appears out of scope for Phase 1:

• Phase 1 focuses on 2-level hierarchy (Actions/Environment only)
• NSM-12 should explore confidence semantics BEFORE implementing specialized semirings
• This TriFold semiring should be part of NSM-12 exploration, not introduced independently

Recommendation: Make this part of an NSM-12 exploration branch for empirical comparison with Product/MinMax semirings.

2. Deviation from BaseSemiring Interface
TriFoldSemiring violates the established contract by operating on 4-tuples instead of scalar confidence values.

Impact:

• Breaks compatibility with existing code expecting BaseSemiring
• Cannot be used interchangeably with ProductSemiring or MinMaxSemiring
• verify_semiring_properties() will fail

Recommendation: Either create a separate TriFoldSemanticReasoner (not inheriting BaseSemiring) or add a ConfidenceAdapter layer.

Major Issues

3. Incomplete Integration Testing
Tests only cover components in isolation. Missing:

• Integration test with SymmetricHierarchicalLayer
• Gradient flow validation
• Batch processing with variable graph sizes
• Cycle consistency with coupling layers

4. Mathematical Rigor and Documentation
Unclear mathematical foundation:

• How does this relate to Scallop-style provenance semirings?
• What are formal properties of fold operator?
• Convergence guarantees for iterative fold/unfold?
• Connection to BDI-HTN-HRL framework?

5. Hyperparameter Proliferation
Adds 6 new hyperparameters without guidance or ablation studies.

Code Quality Issues

6. Type Hints and Shape Documentation
Many functions lack clear shape specifications for tensor parameters.

7. Numerical Instability
Log-space operations without proper clamping could overflow.

8. Inconsistent Naming
TriFoldFold is redundant. Better: TriFoldAggregator and TriFoldBroadcaster.

9. Unused Parameters
In TriFoldReasoner._aggregate, mask parameter ignored in batched case.

10. README Update Premature
TriFold advertised as core feature before validation.

Performance Concerns

11. Computational Overhead
TriFold head adds: projector network, iterative fold/unfold O(iterations × num_nodes), per-node 4-channel ops. Need benchmarks.

12. Memory Usage
fold_history stores all intermediate states. Add store_history=False flag for inference.

Testing Gaps

Missing edge cases: empty graphs, single-node graphs, numerical edge cases, gradient explosion/vanishing.

Positive Aspects

1. Clean code structure with fold/unfold separation
2. TriFoldOutput dataclass pattern
3. Configurable reduction (min/mean/logsumexp)
4. Good type annotations
5. Thorough docstrings

Recommendations

Immediate Actions (Before Merge):

1. Align with roadmap - get approval for Phase 1 scope
2. Fix BaseSemiring compatibility
3. Add integration tests
4. Document mathematics formally
5. Remove README changes until validated

Follow-up Work:

1. Benchmark performance impact
2. Ablation study on hyperparameters
3. Comparison with Product/MinMax on evaluation suite
4. Gradient flow analysis
5. Convergence analysis

Verdict

Status: Request Changes

This is interesting work but introduces significant architectural changes without sufficient justification. Recommend:

Option A (Recommended): Convert to NSM-12 exploration branch for empirical comparison
Option B: Defer to Phase 2 when semantic triple reasoning is in scope
Option C: Provide extensive justification, fix compatibility, add integration tests

Make this part of systematic confidence exploration to compare TriFold vs Product vs MinMax empirically.

Specific Code Comments

nsm/models/confidence/trifold.py:50-52 - Zero/one elements suggest tropical semiring but operations (sum for combine) do not match. Clarify algebraic structure.

nsm/models/hierarchical.py:166 - Consider renaming enable_trifold to enable_tri_semantics for consistency.

tests/models/confidence/test_trifold.py:42 - Add comments explaining expected value calculations.

---

Overall: Ambitious work with potential but needs alignment with project architecture and rigorous validation before integration.

🤖 Generated with Claude Code