Add tri-fold semiring utilities and tests

[research-developer]

Summary

• add a TriFold log-domain semiring with tensor helpers and fold/unfold operators
• expose the tri-fold API alongside documentation in the confidence package
• cover the new functionality with unit tests and stub pytest coverage flags

Testing

• pytest tests/models/confidence/test_trifold.py

---

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

[claude]

PR Review: Add tri-fold semiring utilities and tests

Summary

This PR introduces a TriFoldSemiring implementation with log-domain arithmetic for subject/predicate/object reasoning chains. The implementation includes fold/unfold operators (Φ/Ψ) and comprehensive test coverage.

🟢 Strengths

Code Quality

• Excellent documentation: Module, class, and function docstrings follow Google style with clear mathematical explanations
• Type hints: Proper type annotations throughout (Python 3.10+ union syntax)
• Clean API design: Intuitive naming with mathematical aliases (Phi/Psi) and convenience functions
• Comprehensive __all__ exports: Well-organized public API

Testing

• Good test coverage: 5 test functions covering core functionality
• Clear test names: Descriptive test function names following pytest conventions
• Round-trip validation: Tests verify reversibility (split_trifold round trip)
• Integration testing: Compatibility with verify_semiring_properties

Performance Considerations

• Efficient tensor operations: Uses PyTorch native ops (broadcast_tensors, stack, unbind)
• No unnecessary copies: Operations work in-place where appropriate

---

🟡 Issues & Concerns

1. Architecture Misalignment with CLAUDE.md (HIGH PRIORITY)

Issue: This PR implements a tri-fold semiring before the confidence exploration phase (NSM-12) has been completed.

Per CLAUDE.md lines 5-7 and implementation sequence:

**Current Phase**: Phase 1 Foundation Implementation (2-level hierarchy proof-of-concept)

Per CLAUDE.md lines 318-325:

5. **NSM-10**: Dataset Exploration (3 branches, parallel)
6. **NSM-12**: Confidence Exploration (3 branches, parallel)
   - confidence-product-semiring
   - confidence-minmax-semiring  
   - confidence-learned-aggregation
7. **NSM-11**: Pooling Exploration (3 branches, optional)

Recommendation: This implementation appears to be a fourth confidence approach not listed in the NSM-12 exploration branches. The project should:

1. Decide if TriFold replaces one of the planned explorations OR
2. Add TriFold as a 4th parallel branch for NSM-12 evaluation

Question for maintainer: Was this discussed as part of NSM-12 exploration strategy, or is this a separate experimental direction?

---

2. Mathematical Correctness Issues (HIGH PRIORITY)

Issue 2a: Default dim=-2 is unconventional

Location: trifold.py:94, trifold.py:117

def combine(self, confidences: Tensor, dim: int = -2, **_: object) -> Tensor:
def aggregate(self, confidences: Tensor, dim: int = -2, **_: object) -> Tensor:

Problem:

• BaseSemiring interface uses dim=-1 as default (base.py:49, base.py:79)
• TriFoldSemiring changes default to dim=-2 without clear justification
• This breaks Liskov Substitution Principle - swapping semirings could fail

Impact: Code expecting standard semiring behavior may break:

# This works with ProductSemiring but may fail with TriFoldSemiring
confidences = torch.rand(batch_size, num_paths)  # 2D tensor
result = semiring.combine(confidences)  # expects dim=-1

Recommendation: Either:

• Keep dim=-1 default and document why tri-fold needs special handling, OR
• Add clear documentation explaining the channel-last convention and when to override

---

Issue 2b: Fallback behavior is fragile

Location: trifold.py:109-115

if _is_probability_tensor(confidences):
    eps = torch.finfo(confidences.dtype).tiny
    logs = torch.log(confidences.clamp_min(eps))
    combined = logs.sum(dim=dim)
    return torch.exp(combined)

return confidences.sum(dim=dim)

Problems:

1. Heuristic detection: _is_probability_tensor checks [0,1] bounds, but log-scores can legitimately be in [0,1] (e.g., log(2) ≈ 0.69)
2. Silent behavior change: No warning when switching between log/probability interpretations
3. Final fallback is dangerous: Line 115 does raw .sum() on arbitrary tensors - could produce nonsensical results

Recommendation:

# Option 1: Explicit mode parameter
def combine(self, confidences: Tensor, dim: int = -2, mode: str = "log", **_):
    if confidences.size(-1) == 4:
        # ... tri-fold logic
    elif mode == "probability":
        # ... probability logic
    elif mode == "log":
        return confidences.sum(dim=dim)
    else:
        raise ValueError(f"Unknown mode: {mode}")

# Option 2: Remove fallback, make tri-fold strict
def combine(self, confidences: Tensor, dim: int = -2, **_):
    if confidences.size(-1) != 4:
        raise ValueError("TriFoldSemiring requires 4-channel tensors")
    # ... tri-fold logic only

---

3. Test Coverage Gaps (MEDIUM PRIORITY)

Missing edge cases:

# 1. Empty tensors
scores = torch.empty(0, 4)
semiring.combine(scores, dim=0)  # Untested

# 2. Single-element dimension
scores = torch.rand(1, 4)  # Only 1 step to combine
semiring.combine(scores, dim=0)  # Should this raise or return identity?

# 3. Error conditions
tri = torch.rand(2, 3)  # Wrong shape (not 4 channels)
fold(tri)  # Correctly raises ValueError? (untested)

# 4. Negative indices
scores = torch.rand(2, 3, 4)
semiring.combine(scores, dim=-3)  # Untested

# 5. Invalid reduction names
fold(tri, reduction="invalid")  # Untested
unfold(tri, mode="invalid")  # Untested

# 6. Gradient flow
tri = torch.rand(2, 4, requires_grad=True)
output = Psi_add(Phi_logsumexp(tri))
output.sum().backward()  # Does gradient propagate correctly?

Recommendation: Add tests for:

• test_error_handling_invalid_shapes()
• test_error_handling_invalid_reduction_modes()
• test_gradient_flow_through_fold_unfold()
• test_edge_cases_empty_and_single_element()

---

4. Performance & Numerical Stability (MEDIUM PRIORITY)

Issue 4a: Potential underflow in probability fallback

Location: trifold.py:110-113

eps = torch.finfo(confidences.dtype).tiny  # ~1e-45 for float32
logs = torch.log(confidences.clamp_min(eps))

Problem: torch.finfo().tiny is extremely small (denormal range). For float32:

• tiny ≈ 1e-45
• log(1e-45) ≈ -103.3 (fine)
• But exp(-103.3 * N) for large N → catastrophic underflow

Recommendation: Use a more conservative epsilon:

eps = 1e-8  # Or torch.finfo(confidences.dtype).eps * 10

---

Issue 4b: Missing docstring for internal helpers

Location: trifold.py:49-61

def _ensure_trifold(tensor: Tensor) -> Tensor:
    # No docstring

def _is_probability_tensor(tensor: Tensor) -> bool:
    # No docstring

Recommendation: Add docstrings for maintainability, even for internal functions

---

5. Pytest Configuration Duplication (LOW PRIORITY)

Issue: Coverage stub logic appears in two files:

• conftest.py (lines 1-14)
• tests/pytest_cov_stub.py (lines 1-7)

Problems:

1. DRY violation - same logic in two places
2. tests/__init__.py registers plugin: pytest_plugins = ("tests.pytest_cov_stub",)
3. Root conftest.py also defines pytest_addoption
4. Potential for conflicts or confusing behavior

Recommendation:

• Keep only ONE implementation (prefer conftest.py at root)
• Remove tests/pytest_cov_stub.py and tests/__init__.py:1
• If you need the stub plugin, document WHY (e.g., "Allows running tests without pytest-cov installed")

---

6. API Design Questions (LOW PRIORITY)

Inconsistent naming conventions

• Functions: fold_min, fold_mean, fold_logsumexp (verb_modifier)
• Aliases: Phi_min, Phi_mean, Phi_logsumexp (capital_modifier)
• Modes: Psi_add, Psi_replace, Psi_max (capital_verb)

Recommendation: For consistency, consider:

# Option 1: All lowercase with underscores
phi_min, phi_mean, psi_add, psi_replace

# Option 2: Capitalize mathematical symbols, lowercase for modes
Phi_min, Phi_mean, Psi_add, Psi_replace  # Current (acceptable)

Current naming is acceptable but document the convention in module docstring.

---

🔍 Security Assessment

No security concerns identified. This is pure mathematical/tensor computation code with no:

• External I/O
• Credential handling
• Code execution
• Network access

---

📊 Performance Benchmarks (Recommended)

Consider adding performance tests for large-scale scenarios:

def test_performance_large_batch():
    """Ensure fold/unfold scales to realistic batch sizes."""
    batch_size = 10000
    tri = torch.rand(batch_size, 4)
    
    import time
    start = time.time()
    result = Psi_add(Phi_logsumexp(tri))
    elapsed = time.time() - start
    
    assert elapsed < 0.1, f"Too slow: {elapsed:.3f}s for {batch_size} items"

---

✅ Actionable Recommendations

Must Fix (Before Merge)

1. Clarify NSM-12 alignment: Document how this fits into confidence exploration strategy
2. Fix dim default: Align with BaseSemiring contract (dim=-1) or document breaking change
3. Add error handling tests: Invalid shapes, modes, reductions
4. Fix epsilon value: Use 1e-8 instead of torch.finfo().tiny

Should Fix (Before Merge)

5. Remove pytest stub duplication: Keep conftest.py OR tests/pytest_cov_stub.py
6. Improve fallback logic: Make mode explicit or remove auto-detection
7. Add gradient tests: Verify backpropagation through Phi/Psi

Nice to Have (Future PR)

8. Performance benchmarks: Add timing tests for large batches
9. Extended documentation: Add usage examples to module docstring
10. Integration example: Show TriFold in a multi-hop reasoning scenario

---

🎯 Verdict

Conditional Approval ✅ (pending clarifications)

The code quality is high and the mathematical approach is sound, but there are alignment questions with the project roadmap (NSM-12) and some API design issues that should be resolved before merge.

Key Questions for Author:

1. How does this relate to NSM-12 confidence exploration branches?
2. Why change dim default from -1 to -2?
3. Should we remove the probability fallback and make TriFold strict about log-domain inputs?

---

Reviewed by: Claude Code
Review Date: 2025-10-21
Based on: CLAUDE.md project guidelines and NSM Phase 1 architecture