Skip to content

Fix HardShrink to accept its documented lambd argument#3786

Open
Pablosinyores wants to merge 1 commit into
ml-explore:mainfrom
Pablosinyores:fix/hardshrink-lambd
Open

Fix HardShrink to accept its documented lambd argument#3786
Pablosinyores wants to merge 1 commit into
ml-explore:mainfrom
Pablosinyores:fix/hardshrink-lambd

Conversation

@Pablosinyores

Copy link
Copy Markdown
Contributor

What

nn.HardShrink documents a configurable lambd argument (Default: 0.5), but the class cannot actually be constructed with one:

nn.HardShrink(lambd=0.3)
# TypeError: Module.__init__() got an unexpected keyword argument 'lambd'
nn.HardShrink(0.3)
# TypeError: ... takes 1 positional argument but 2 were given

The class is built with @_make_activation_module(hard_shrink), which sets __call__ to always apply the default lambd and leaves the class with no __init__ (it inherits Module.__init__(self)). So the documented parameter is silently unusable.

Fix

Replace the decorator with an explicit __init__/__call__ that stores and forwards lambd, mirroring the sibling Softshrink layer (which already does this and is tested). Every other parameterized activation (Softshrink, ELU, CELU, LeakyReLU, PReLU, Step) follows this same pattern; HardShrink was the lone one mistakenly decorated.

Tests

Extended test_hard_shrink to exercise the class path (nn.HardShrink() and nn.HardShrink(lambd=0.1)), which was previously untested — only the functional nn.hard_shrink was covered, which is why the missing parameter went unnoticed.

HardShrink was built with @_make_activation_module, which sets
__call__ to always use the default lambd and gives the class no
__init__. Its docstring documents a configurable lambd, but the class
could not be constructed with one: nn.HardShrink(lambd=0.3) raised
TypeError. Replace the decorator with an explicit __init__/__call__
that stores and forwards lambd, mirroring the sibling Softshrink layer.

Extend test_hard_shrink to exercise the class path (default and
lambd=0.1), which was previously untested.

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

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.

2 participants