Skip to content

Epsilon-scaled clipped versions of Softmax and Sigmoid as utilities - #444

Merged
jpchen merged 7 commits into
devfrom
clipped-nn-ops
Nov 3, 2017
Merged

Epsilon-scaled clipped versions of Softmax and Sigmoid as utilities#444
jpchen merged 7 commits into
devfrom
clipped-nn-ops

Conversation

@rohitsingh0812

@rohitsingh0812 rohitsingh0812 commented Nov 1, 2017

Copy link
Copy Markdown
Collaborator

Blocking #107

Epsilon-scaled clipped versions of Softmax and Sigmoid as utilities that can be used to avoid numerical unstability with Pytorch.nn Softmax and Sigmoid operations

Tested

Added simple tests for both new classes.

@fritzo fritzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please fix lint errors:

./pyro/nn/clipped_nn.py:2:1: F401 'torch' imported but unused
./pyro/nn/clipped_nn.py:5:1: E302 expected 2 blank lines, found 1
./pyro/nn/__init__.py:5:1: F401 '.clipped_nn.ClippedSigmoid' imported but unused
./pyro/nn/__init__.py:5:1: F401 '.clipped_nn.ClippedSoftmax' imported but unused
./pyro/nn/__init__.py:5:55: W292 no newline at end of file

Comment thread pyro/nn/clipped_nn.py Outdated

def forward(self, val):
rval = super(ClippedSoftmax, self).forward(val)
return (rval * (1.0 - 2 * self.epsilon)) + self.epsilon

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should really be

n = rval.shape(self.dim)
return (rval * (1.0 - n * self.epsilon)) + self.epsilon

so that the clipped value is still normalized and so gradients still sum to zero.

Comment thread pyro/nn/clipped_nn.py Outdated

def forward(self, val):
rval = super(ClippedSoftmax, self).forward(val)
n = rval.shape(self.dim)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where does the dim here come from? Is it an attribute of nn.Softmax?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@fritzo - that'll be there in the next release, but its not in the current release. Maybe we should add a comment to that effect?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so maybe getattr(self, 'dim', -1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that should work!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be size rather than shape

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Note that Tensor.shape is not available on PyTorch 0.2 release, whereas Tensor.size() is)

@neerajprad

neerajprad commented Nov 2, 2017

Copy link
Copy Markdown
Member

Can we also have some simple tests for the clipped versions of sigmoid and softmax?

Comment thread pyro/nn/clipped_nn.py


class ClippedSoftmax(nn.Softmax):
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Outdent comment and wrap code in `` to satisfy sphinx:

    """
    A wrapper around `nn.Softmax` that scales its output
    from `[0,1]` to `[epsilon,1-epsilon]`.
    """

Also fix below comment. Note that this is important since we've had sphinx errors due to code not wrapped in ticks.

@rohitsingh0812 rohitsingh0812 mentioned this pull request Nov 2, 2017
@fritzo

fritzo commented Nov 2, 2017

Copy link
Copy Markdown
Member

Looks good. This just needs some simple tests like

def test_clipped_softmax():
    epsilon = 1e-5
    clipped_softmax = ClippedSoftmax(epsilon, dim=0)
    ps = Variable(torch.Tensor([0, 1, 2]))
    softmax_ps = clipped_softmax(ps)
    assert (softmax_ps.data >= epsilon).all(), (softmax_ps, epsilon)
    assert_equal(softmax_ps.data.sum(), 1.0)

@fritzo

fritzo commented Nov 2, 2017

Copy link
Copy Markdown
Member

@rohitsingh0812 I've added some tests. Could you PTAL?

@jpchen jpchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

merging due to peer pressure

@jpchen
jpchen merged commit da8f6b4 into dev Nov 3, 2017
@jpchen
jpchen deleted the clipped-nn-ops branch November 3, 2017 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants