Skip to content

Store _prev as a tuple in sum() and mean() - #18

Merged
Terminay merged 1 commit into
Terminay:mainfrom
eeshsaxena:fix-reduction-prev-tuple
Aug 8, 2026
Merged

Store _prev as a tuple in sum() and mean()#18
Terminay merged 1 commit into
Terminay:mainfrom
eeshsaxena:fix-reduction-prev-tuple

Conversation

@eeshsaxena

Copy link
Copy Markdown
Contributor

Fixes #7.

The bug

Every op builds its _prev through _create_child, which normalises the argument with tuple(prev). sum() and mean() skip that helper and assign the set directly:

out._prev = {self}

so those two nodes carry a set while every other node carries a tuple.

_eval_forward pulls operands by index:

elif node._op == "sum":
    values[node] = values[node._prev[0]].sum()

Indexing a set raises TypeError: 'set' object is not subscriptable, so evaluating any graph that reduces with sum or mean (which is almost every loss) crashes.

The fix

Store (self,) in both methods so _prev is a tuple everywhere:

out._prev = (self,)

Test

Added test_eval_forward_handles_reductions, which runs a sum graph and a mean graph through _eval_forward and checks the values (30.0 and 14/3). It raises TypeError on main and passes with this change. The full tests/test_gradcheck.py suite passes locally.

One heads-up while I was in there: grad_check has a separate, unrelated problem where it perturbs the reduced output node (node.data[idx] = ...) whose data is a scalar, which raises 'numpy.float64' object does not support item assignment. That is outside the scope of this fix, so I left it alone, but happy to open a follow-up if useful.

Every op builds its _prev through _create_child, which normalises it with
tuple(prev). sum() and mean() instead set out._prev = {self} directly, so
those two nodes carried a set.

_eval_forward reads node._prev[0] to pull an operand, and indexing a set
raises "TypeError: 'set' object is not subscriptable". So evaluating any graph
that reduces with sum or mean, which is almost every loss, crashed.

Store (self,) in both methods so _prev is a tuple everywhere. Added a test
that evaluates a sum and a mean graph through _eval_forward.
@Terminay

Terminay commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Hi! Welcome to Leanpass, @eeshsaxena; this is a genuinely huge milestone for me. You are leanpass's FIRST contributor!!

@Terminay
Terminay merged commit 716c05c into Terminay:main Aug 8, 2026
3 checks passed
@Terminay

Terminay commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Good catch about grad_check turns out #17 doesn't actually cover the scalar item-assignment crash you found, so it's a separate thing after all. Could you open a new issue for it? Your write-up above is basically issue-ready as-is; feel free to just copy it over. Happy to have you pick it up too if you want, but no pressure either way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent _prev container type breaks graph traversal

2 participants