fix: metrics layer cleanliness + matrix.py doc corrections - #43
Conversation
- compute.py: remove unused prev_energy param from energy_step and its call site; fix _compute_D docstring to reflect 1-cosine_sim impl; remove unused Counter/math/BoneToken imports; change RoundMetrics round_index/token_count/bone_count defaults from 0.0 to 0 (int) - projection.py: remove dead counts=Counter(...) variable in gini_tbf; add elif/ValueError for unknown direction in fire_alerts - risk.py: wrap broken_return return value in clamp() for consistency - matrix.py: update kappa docstring to kappa in [0,1]; add note that A_MATRIX is documentation-shaped and should become single source of truth; add Layer-mixing comment on E entry Closes #38, #39
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5e2da8aa
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR performs a small “cleanliness” pass across the metrics layer and corrects documentation around κ (kappa) and the metric matrix/projection definitions, addressing issues #38 and #39.
Changes:
- Simplifies the RC-circuit helper by removing an unused
prev_energyargument fromenergy_step()and updates the call site accordingly. - Removes dead/unused code in projection/risk helpers and hardens alert firing by supporting
"below"thresholds and raising on unknown directions. - Updates
matrix.pydocumentation to correctly describe κ ∈ [0, 1] and clarifies thatA_MATRIXis currently documentation-only (not used bycompute.py).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/src/edcmbone/metrics/compute.py | Removes unused energy_step parameter, cleans imports, adjusts defaults, and fixes _compute_D docstring. |
| backend/src/edcmbone/metrics/projection.py | Removes dead variable in gini_tbf and adds "below" + validation handling in fire_alerts. |
| backend/src/edcmbone/metrics/risk.py | Wraps broken_return output (and formula doc) in clamp() for consistency. |
| backend/src/edcmbone/metrics/matrix.py | Corrects κ range documentation and adds notes clarifying the current A_MATRIX vs implementation divergence. |
Comments suppressed due to low confidence (1)
backend/src/edcmbone/metrics/compute.py:131
- The module docstring still states stored tension
kis "≥ 0" (unbounded), butenergy_step()clampskappaviaclamp(...)which restricts it to [0, 1] (andmatrix.pynow documents κ ∈ [0, 1]). Please update the module docstring’s k/kappa range to match the implementation.
def energy_step(prev_kappa, dissonance, alpha=0.85, delta_max=0.3):
"""Compute one step of the RC-circuit energy model.
s_{t+1} = alpha * s_t + E_t - delta_t
delta_t = min(delta_max, g(y_t, y_{t-1}))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
energy_step(0.0, 0.0, dissonance=0.9, ...) passed prev_energy as the second positional arg, which now maps to dissonance — causing TypeError on the keyword duplicate. Drop the extra 0.0; new call is energy_step(0.0, 0.9, alpha=0.9, delta_max=0.1). Addresses Codex review comment on #43.
- Remove prev_energy=0.0 from compute_round signature and docstring - Remove prev_energy plumbing from compute_transcript (init + kwarg pass) - Module docstring: update k description from '>= 0' to 'kappa in [0,1]' to match the clamp() applied in energy_step Addresses Copilot review comment on PR #43 and erinepshovel-code @codex repair request (line 127).
…script plumbing Agent-Logs-Url: https://github.com/The-Interdependency/edcmbone/sessions/b891bdfe-243e-4c21-a39d-ba7b085c021f Co-authored-by: erinepshovel-code <250928284+erinepshovel-code@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4294387457
ℹ️ 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".
| def compute_round(round_, prev_round=None, canon=None, | ||
| alpha=0.85, delta_max=0.3, | ||
| prev_kappa=0.0, prev_energy=0.0, prev_entropy=0.0): | ||
| prev_kappa=0.0, prev_entropy=0.0): | ||
| """Compute the metric vector for a Round. |
There was a problem hiding this comment.
Keep
compute_round compatible with prev_energy callers
Removing prev_energy from compute_round is a backward-incompatible API change for existing integrations that still pass prev_energy= (the parameter existed in the prior signature and this function is exported via edcmbone.metrics). In this commit those calls now fail immediately with TypeError: unexpected keyword argument 'prev_energy', even though the value was previously ignored, so preserving it as an optional no-op argument would avoid breaking downstream code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Restored backward compatibility in
compute_roundby reintroducing an optionalprev_energyparameter so downstream callers that still passprev_energy=no longer fail withTypeError. The value is accepted as a no-op. - Updated the
compute_rounddocstring to explicitly documentprev_energyas a deprecated compatibility argument that is accepted but ignored. - Committed the fix on the current branch as commit
2735ef4and created a follow-up PR message viamake_prper instructions.
Testing
- ✅
python -m py_compile backend/src/edcmbone/metrics/compute.py
|
@copilot apply changes based on the comments in this thread |
Addresses issues #38 and #39.
Changes
compute.py (issue #39)
prev_energyparameter fromenergy_stepand updated the one call site incompute_roundaccordingly. The parameter was never read inside the function body._compute_Ddocstring: was "low bone density as proxy", now accurately describes the1 - cosine_simimplementation.Counter,math, andBoneToken.RoundMetrics.__init__defaults forround_index,token_count, andbone_countfrom0.0(float) to0(int).projection.py (issue #39)
counts = Counter(t.token_count for t in turns)ingini_tbf— it was built and immediately discarded; the real work usesspeaker_counts.elif direction == "below"branch and aValueErrorfor unknown directions infire_alerts, so a future"below"threshold fires correctly and any unexpected direction value raises immediately rather than silently not firing.risk.py (issue #39)
broken_return's return value inclamp()for consistency with every other risk function. The output is already in [0, 1] by construction, so this is a no-op safety wrapper.matrix.py (issue #38)
energy_step()clamping andPROJECTION_MAP["DA"]requiring it.A_MATRIXis currently documentation-shaped (actual weights are hardcoded incompute.py) and should eventually become the single source of truth.# NOTE: E depends on Layer 1 metric R — Layer mixing; see issue #38comment on theA_MATRIX["metrics"]["E"]entry.Test plan
pytest backend/tests/metrics/energy_stepcall sites pass only(prev_kappa, dissonance, alpha, delta_max)— noprev_energy.fire_alertswith a"below"direction spec fires as expected.broken_returnreturn value is still in [0, 1] (clamp is a no-op but now explicit).Closes #38
Closes #39
Generated by Claude Code