Skip to content
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

Fix setattr issue in canonizers.MergeBatchNorm (#208) #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karray
Copy link

@karray karray commented Jul 19, 2024

Fixes #208

Copy link
Owner

@chr5tphr chr5tphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR.

There remains a potential issue: Since we use the torch.nn.Module's setattr to register a Parameter if it did not exist before, the Parameter may still be registered after the Canonizer is removed, since we set the attribute back to None without using the Module's setattr, which would detect this change. We need to explicitly unregister the Parameter, i.e., by using register_parameter or the Module's setattr explicitly only if the original bias was None in the Canonizer's remove().

src/zennit/canonizers.py Show resolved Hide resolved
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.

Unexpected behavior with object.__setattr__ in canonizers.MergeBatchNorm
2 participants