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

Test statistics of initializers #5511

Merged
merged 10 commits into from Jan 8, 2019

Conversation

toslunar
Copy link
Member

@toslunar toslunar commented Oct 18, 2018

Test scale arg of chainer.initializers.

This PR adds statistical tests and fixes bias of sign of Orthogonal initializer.

@toslunar toslunar added cat:bug Bug report or fix. cat:test Test or CI related. labels Oct 18, 2018
@toslunar
Copy link
Member Author

@sunaemon Could you review the math part of this PR?

@toslunar toslunar added to-be-backported Pull request that should be backported. and removed cat:bug Bug report or fix. labels Nov 2, 2018
@toslunar
Copy link
Member Author

toslunar commented Nov 2, 2018

This PR can be backported because the change to Orthogonal is moved to #5615.

@mitmul
Copy link
Member

mitmul commented Nov 2, 2018

Will you make the test case for the orthogonal initializer stricter by removing abs over the sample values after this PR is merged and backported for v5? Or isn't it necessaliry be such a strict condition for testing the orthogonal initializer?

@toslunar
Copy link
Member Author

toslunar commented Nov 2, 2018

#5615 makes the test stricter by removing abs.

@sunaemon
Copy link
Contributor

sunaemon commented Nov 6, 2018

@toslunar
The statistical tests for orthogonal initializer seem verifies a necessary condition!

Sorry for the delay.

@mitmul
Copy link
Member

mitmul commented Nov 22, 2018

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit e792034, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@toslunar
Copy link
Member Author

A test (with alpha=0.05 and repeat_with_success_at_least(5, 3)) fails with probability 0.001158125. This seems too large to have parameterized tests.

@toslunar
Copy link
Member Author

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 44b63fc, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@mitmul
Copy link
Member

mitmul commented Dec 10, 2018

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 44b63fc, target branch master) succeeded!

@okuta
Copy link
Member

okuta commented Dec 24, 2018

@mitmul Test was passed.

@okuta okuta added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Dec 24, 2018
@okuta okuta added this to the v6.0.0b2 milestone Dec 24, 2018
@mitmul mitmul merged commit 602d1fb into chainer:master Jan 8, 2019
mitmul added a commit to mitmul/chainer that referenced this pull request Jan 8, 2019
@toslunar toslunar deleted the test-initializers-stats branch January 9, 2019 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:test Test or CI related. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. to-be-backported Pull request that should be backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants