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

Add reduce option to decov #2698

Merged
merged 3 commits into from May 8, 2017
Merged

Add reduce option to decov #2698

merged 3 commits into from May 8, 2017

Conversation

delta2323
Copy link
Member

@delta2323 delta2323 commented May 3, 2017

This PR adds reduce option to F.Decov and F.decov. Related to #2558

@delta2323 delta2323 added the cat:feature Implementation that introduces new interfaces. label May 3, 2017
Copy link
Member

@unnonouno unnonouno left a comment

Choose a reason for hiding this comment

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

Check comments

cost = xp.vdot(self.covariance, self.covariance) * h.dtype.type(0.5)
return utils.force_array(cost),
if self.reduce == 'half_frobenius_norm':
cost = xp.vdot(self.covariance, self.covariance)
Copy link
Member

@unnonouno unnonouno May 7, 2017

Choose a reason for hiding this comment

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

Is sqrt required for frobenius norm, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. I think it is better to provide squared norm (like MSE). So, I change the option to 'half_squared_frobenius_norm'.

Args:
h (Variable): Variable holding a matrix where the first dimension
corresponds to the batches.
recude (str): Reduction option. Its value must be either
Copy link
Member

Choose a reason for hiding this comment

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

reduce

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix

loss_shape = ()
else:
loss_shape = (3, 3)
self.assertEqual(loss.data.shape, loss_shape)
Copy link
Member

Choose a reason for hiding this comment

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

Use self.gloss.shape

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix

@unnonouno unnonouno self-assigned this May 7, 2017
Copy link
Member

@unnonouno unnonouno left a comment

Choose a reason for hiding this comment

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

I feel 'half_squared_frobenius_norm' is long.

@@ -10,9 +10,14 @@ class DeCov(function.Function):

"""DeCov loss (https://arxiv.org/abs/1511.06068)"""

def __init__(self):
def __init__(self, reduce='half_squared_frobenius_norm'):
Copy link
Member

Choose a reason for hiding this comment

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

How about 'half_squared_sum'?

@unnonouno unnonouno merged commit 9a44d5f into master May 8, 2017
@unnonouno unnonouno deleted the decov-wo-reduction branch May 8, 2017 08:42
@unnonouno
Copy link
Member

LGTM

@beam2d beam2d added this to the v1.24.0 milestone May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature Implementation that introduces new interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants