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.

Sign up for GitHub

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

Jump to bottom

Fix Adam FP16 overflow on gpu kernels #7694

Merged
merged 5 commits into from
Jul 19, 2019
Merged

Fix Adam FP16 overflow on gpu kernels #7694

merged 5 commits into from
Jul 19, 2019

Conversation

emcastillo
Copy link
Member

Fix #7690

Found that it was due to lack of precision as some values were not being converted to FP32 during intermediate calculations.

@emcastillo emcastillo added cat:test Test or CI related. cat:bug Bug report or fix. labels Jul 4, 2019
@niboshi niboshi removed the cat:test Test or CI related. label Jul 5, 2019
@niboshi niboshi self-assigned this Jul 5, 2019
@niboshi
Copy link
Member

niboshi commented Jul 9, 2019

Jenkins, test this please

@niboshi niboshi changed the title [FlakyTests] Fix Adam FP16 overflow on gpu kernels Fix Adam FP16 overflow on gpu kernels Jul 9, 2019
@niboshi niboshi added cat:enhancement Implementation that does not break interfaces. and removed cat:bug Bug report or fix. labels Jul 9, 2019
@niboshi niboshi added this to the v7.0.0b2 milestone Jul 9, 2019
@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jul 9, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 0d65b03, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Jul 9, 2019

Please check the test failures.

@niboshi niboshi added st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. and removed st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Jul 9, 2019
@emcastillo
Copy link
Member Author

emcastillo commented Jul 10, 2019 •
edited
Loading

There was a lack of conversions back for m, v and vhat

@niboshi niboshi removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jul 16, 2019
niboshi
niboshi requested changes Jul 16, 2019
View reviewed changes
chainer/optimizers/adam.py Outdated Show resolved Hide resolved
@niboshi niboshi added the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jul 16, 2019
@niboshi niboshi removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jul 18, 2019
@niboshi
Copy link
Member

niboshi commented Jul 18, 2019

Jenkins, test this please

@asi1024 asi1024 removed this from the v7.0.0b2 milestone Jul 18, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 9184bfd, target branch master) failed with status FAILURE.

niboshi
niboshi requested changes Jul 18, 2019
View reviewed changes
chainer/optimizers/adam.py Outdated Show resolved Hide resolved
@niboshi
Copy link
Member

niboshi commented Jul 18, 2019

Jenkins, test this please

@niboshi niboshi added this to the v7.0.0b3 milestone Jul 18, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 1eed627, target branch master) failed with status FAILURE.

@niboshi
Copy link
Member

niboshi commented Jul 18, 2019

Jenkins failure is unrelated.

@niboshi
Copy link
Member

niboshi commented Jul 19, 2019

LGTM

@niboshi niboshi merged commit 02d1f0c into chainer:master Jul 19, 2019
@niboshi niboshi added the to-be-backported Pull request that should be backported. label Jul 19, 2019
@niboshi niboshi mentioned this pull request Jul 19, 2019
Merged
niboshi added a commit to niboshi/chainer that referenced this pull request Jul 19, 2019
@niboshi
Merge pull request chainer#7694 from emcastillo/fix_adam_test
98ed687 
Fix Adam FP16 overflow on gpu kernels
@emcastillo emcastillo deleted the fix_adam_test branch July 19, 2019 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niboshi niboshi niboshi approved these changes

Assignees

@niboshi niboshi

Labels
cat:enhancement Implementation that does not break interfaces. 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
Milestone
v7.0.0b3
Development

Successfully merging this pull request may close these issues.

Flaky test: TestAdam
4 participants
@emcastillo @niboshi @chainer-ci @asi1024