Fix AbstractFeature.degrade calling itself instead of degrade_function - #792
Fix AbstractFeature.degrade calling itself instead of degrade_function#792Anai-Guo wants to merge 1 commit into
Conversation
…of degrade_function degrade() ended in `return self.degrade(self.value)`, so the first call raises TypeError: AbstractFeature.degrade() takes 1 positional argument but 2 were given. Degradation.compute already has the correct form, `return self.degrade_function(self.value)`. Signed-off-by: Tai An <antai12232931@outlook.com>
|
Thanks @Anai-Guo — your diagnosis is exactly right, and I confirmed every part of it. I went one step further than the fix, for the reason you raised yourself at the end of your description. Two things decided it. First, It looks like an intended third per-feature lifecycle hook alongside So #793 removes the method instead. Closing this in favour of that, but the bug is yours — the PR credits you and links back here. Please do send more; this was a good catch and a well-written report. |
AbstractFeature.degrade called itself with an argument it does not accept, so it raised TypeError on the first call for every feature. It has been that way since 4b577e9 added topology_features.py, and nothing in the repo has ever called it. It looks like an intended third per-feature lifecycle hook alongside update and normalize, which MulticompartmentConnection does call over its pipeline. The matching 'for f in self.pipeline: f.degrade()' loop was never written; degradation lives in Degradation.compute instead, which is where the pipeline applies it and which already guards against degrade_function being None. Fixing the method would leave two spellings of the same operation, one of them unguarded, so remove it. Reported by @Anai-Guo in #792. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Problem
AbstractFeature.degradecalls itself:degradetakes no arguments afterself, so this is not even infinite recursion — it raises on the very first call, for every feature, includingDegradation:No other class overrides
degrade, so there is no code path on which it works.Fix
The docstring describes exactly the callable that
Degradation.__init__stores asdegrade_function("a lambda ... which takes a single argument (which represent the value), and returns a value which will be subtracted from the propagated spikes"), andDegradation.computealready contains the intended form a few hundred lines below:So this is a one-word slip:
self.degrade(...)→self.degrade_function(...). One line changed, plus a regression test next to the existingtest_degradation_feature_output.The new test fails on
masterwith theTypeErrorabove and passes with the fix.One thing worth your call
degrade_functiononly exists onDegradation, so callingdegrade()on some other feature (Weight,Mask, …) now raisesAttributeErrorinstead of the currentTypeError. That seemed like the smaller change to make, but if you would rather the method live onDegradation— or guard withif self.degrade_function is not Nonethe waycomputedoes — say the word and I will move it.🤖 Generated with Claude Code