-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stopgap make_zero bugfix #1961
base: main
Are you sure you want to change the base?
Stopgap make_zero bugfix #1961
Conversation
Aiming for full coverage of both new and old implementations of make_zero(!)
EnzymeCore.make_zero(Core.Typeof(prev2), seen, prev2, Val(copy_if_inactive)), | ||
) | ||
seen[prev] = res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ordering feels weird, what if a box is in a recursive datastructure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't see what's wrong? The first time you encounter this prev
, wherever it lives in the datastructure, you construct the corresponding zeroed value, and then you store it in the IdDict such that if the same identical prev
is encountered again, make_zero
will return the same identical res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean now, self-referential data structures hadn't even crossed my mind while working on this. Will make sure to add tests for that to whichever PR gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual point of the change here was to get rid of the spurious Ref. The reordering was collateral.
return prev | ||
# Unclear what types might end up here rather than in specialized methods or | ||
# guaranteed_const_nongen, but as a last-ditch attempt try falling back to Base.zero | ||
return Base.zero(prev)::RT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it has no fields to zero, we should use the old object here
Thanks for taking a look! I noticed a few other issues that should be fixed too. However, #1852 is almost there (I've been in a cycle of thorough tests => noticing issues => fixing/improving design accordingly => repeat), so if you don't mind I'll finish the big push on that, and then we can decide whether it's worth fixing and merging this one first (say, if reviewing #1852 is too big of a task to deal with right now), or if we should close this one and focus on #1852 |
Since #1852 isn't quite ready, in the meantime, here's a PR fixing the bugs I found in make_zero(!) and adding a fairly thorough test suite.
The last test currently fails due to #1935. I didn't want to mark it as broken because I'd rather see #1936 merged (or #1935 fixed in some other way), but let me know if I should change that.