fix(memory): apply per_hop_decay once per hop in _spread 🤖🤖🤖 - #297
fix(memory): apply per_hop_decay once per hop in _spread 🤖🤖🤖#297sushant-mishra-dtu wants to merge 1 commit into
Conversation
_spread's docstring says it propagates activation "decaying per_hop_decay
per hop", and the config comment agrees: "delta -- activation decay per
hop". The loop multiplies by ``per_hop_decay ** h`` at hop h, but the
activation it multiplies is the frontier value, which already carries the
decay of every earlier hop. The factors compound to delta ** (h(h+1)/2).
On a chain of unit-weight causal edges with the defaults
(per_hop_decay=0.6, activation_floor=0.05):
hop measured documented delta**h
1 0.600000 0.600000
2 0.216000 0.360000
3 DROPPED 0.216000
Hop 3 falls under activation_floor and is discarded, so a memory three
causal edges from the seed is never recalled however strong the links.
The frontier already carries the earlier hops, so apply one factor per
step. Defaults are unaffected -- RetrievalConfig.hops is 1, where both
forms give delta -- this only changes the documented "2+ = multi-hop"
path, which now matches the documented rate.
🤖🤖🤖
Signed-off-by: sushant-mishra-dtu <sushant.arh@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe associative spread loop now applies ChangesAssociative spread decay
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to Multi-hop associative retrieval now retains activation according to one decay factor per hop, improving intended recall behavior without changing single-hop behavior. The corrected decay sequence is covered by a three-hop regression test, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What this fixes
RetrievalEngine._spreadis documented as decaying once per hop, in the docstring(
packages/nooa-memory/src/nooa_memory/retrieval.py:249):and again on the knob itself (
packages/nooa-memory/src/nooa_memory/config.py:35):The loop applies
per_hop_decay ** hat hoph(
packages/nooa-memory/src/nooa_memory/retrieval.py:265-283):But
actcomes fromfrontier, which is last hop'snxt-- it already carries the decayof every earlier hop. Multiplying by
delta ** hagain compounds them: the total appliedat hop
hisdelta ** (1+2+...+h), i.e.delta ** (h(h+1)/2), notdelta ** h.Why it matters
The compounding collapses fast enough to cross
activation_floorand silently discardwhole hops. On a chain of unit-weight
CAUSESedges with the shipped defaults(
per_hop_decay=0.6,activation_floor=0.05):maindelta ** hA memory three causal edges from the seed is never recalled, however strong the links --
hops=3behaves ashops=2at the defaults. At hop 2 it is not dropped, just scored 40%low, which quietly moves it down the ranking against unlinked candidates.
RetrievalConfig.hopsis documented as0 = vector-only; 1 = +1 graph hop; 2+ = multi-hop(
config.py:34), so2+is a supported configuration, not an edge case.Reproduction
Four memories in a chain
a -> b -> c -> d, allEdgeType.CAUSES, all weight 1.0:main:this branch:
The fix
The frontier already carries the earlier hops, so apply one factor per step:
spread: dict[str, float] = {} frontier = dict(seed_activation) - for h in range(1, hops + 1): - decay = cfg.per_hop_decay**h + # ``act`` already carries the decay of every earlier hop, so applying a + # single factor per step is what yields ``per_hop_decay ** h`` at hop h. + decay = cfg.per_hop_decay + for _ in range(hops): nxt: dict[str, float] = {}The behaviour change, stated plainly: multi-hop spread values go up, so recall results
can reorder for anyone running
hops >= 2. At the defaulthops=1nothing changes at all--
delta ** 1isdeltaunder either form. No existing test inpackages/nooa-memory/tests/memory/changes result; the one that covers this(
test_spread_decays_per_hop) asserts onlyspread[b] > spread[c] > 0, which is why therate was never pinned down. If you would rather keep today's numbers and correct the two
comments instead, say so and I will send that diff -- but at the defaults that means
documenting
hops=3as having no third hop.Test
One test,
test_spread_decay_is_one_factor_per_hop, placed beside the existingtest_spread_decays_per_hopit strengthens. Verified to fail on the unfixed tree beforebeing kept:
packages/nooa-memory/tests/memory/is 3 failed / 267 passed / 13 skipped on this branchagainst 3 failed / 266 passed / 13 skipped on
main-- the same three pre-existingWindows-only failures (two
read_text()calls with noencoding=inside the test files,one expecting
/etc/passwdto be absolute), plus the new test passing.Worth noting: that directory is not in
testpaths, so CI does not currently collect thistest. #292 is the one-line change that turns it on.
Scope
Only the decay factor.
_visibleand the owner-scoping logic are untouched, so this doesnot overlap #275.
Summary by CodeRabbit
Bug Fixes
Tests