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 optimizer_hooks.GradientHardClipping for scalar array #7760

Merged
merged 3 commits into from
Jul 19, 2019
Merged

Fix optimizer_hooks.GradientHardClipping for scalar array #7760

merged 3 commits into from
Jul 19, 2019

Conversation

niboshi
Copy link
Member

@niboshi niboshi commented Jul 12, 2019 •
edited
Loading

Fixes #7759

This PR was originally intended to refactor the tests (#7759) but I found a bug in optimizer_hooks.GradientHardClipping. I can split the PR if it's preferable.

@niboshi niboshi force-pushed the refactor-optimizer-hook-test branch from 1fca7d5 to 1ee51c1 Compare July 12, 2019 08:56
@niboshi niboshi changed the title [WIP] Refactor dummy links in optimizer_hooks_tests Fix optimizer_hooks.GradientHardClipping for scalar array Jul 12, 2019
@emcastillo emcastillo self-assigned this Jul 16, 2019
emcastillo
emcastillo previously approved these changes Jul 16, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approach LGTM, but fix CI issues first

@emcastillo
Copy link
Member

The bug-fix is pretty small and related to the tests.
I think is ok to merge both of them together.

@emcastillo emcastillo added cat:code-fix Code refactoring that does not change the behavior. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Jul 16, 2019
@emcastillo emcastillo added this to the v7.0.0b2 milestone Jul 16, 2019
@emcastillo emcastillo removed the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jul 16, 2019
@emcastillo emcastillo self-requested a review July 16, 2019 06:31
@emcastillo emcastillo dismissed their stale review July 16, 2019 06:32

not approved

@niboshi niboshi force-pushed the refactor-optimizer-hook-test branch from 1ee51c1 to f03d5ba Compare July 16, 2019 06:41
@niboshi
Copy link
Member Author

niboshi commented Jul 18, 2019

Fixed errors.

@emcastillo
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

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

@asi1024 asi1024 removed this from the v7.0.0b2 milestone Jul 18, 2019
emcastillo
emcastillo approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo
Copy link
Member

Jenkins, test this please.

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

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

@emcastillo
Copy link
Member

Jenkins, test this please.

@chainer-ci
Copy link
Member

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

@emcastillo
Copy link
Member

CI unrelated

@beam2d beam2d added the ChainerX-long Long running PR for ChainerX task board label Jul 19, 2019
@emcastillo emcastillo merged commit f8be8c7 into chainer:master Jul 19, 2019
@emcastillo emcastillo removed the ChainerX-long Long running PR for ChainerX task board label Jul 19, 2019
@niboshi niboshi added cat:bug Bug report or fix. to-be-backported Pull request that should be backported. and removed cat:code-fix Code refactoring that does not change the behavior. labels Jul 19, 2019
@niboshi
Copy link
Member Author

niboshi commented Jul 19, 2019

@emcastillo
This is a bugfix. Can you backport?

@niboshi niboshi deleted the refactor-optimizer-hook-test branch July 19, 2019 05:00
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

3 similar comments
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@niboshi
Copy link
Member Author

niboshi commented Aug 13, 2019

@emcastillo Can you make a backport?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

10 similar comments
@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@chainer-ci
Copy link
Member

@emcastillo This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@emcastillo emcastillo mentioned this pull request Nov 5, 2019
Merged
emcastillo pushed a commit to emcastillo/chainer that referenced this pull request Nov 5, 2019
Merge pull request chainer#7760 from niboshi/refactor-optimizer-hook-…
8612cf6 
…test

Fix `optimizer_hooks.GradientHardClipping` for scalar array
emcastillo pushed a commit to emcastillo/chainer that referenced this pull request Nov 18, 2019
Merge pull request chainer#7760 from niboshi/refactor-optimizer-hook-…
dbf1aba 
…test

Fix `optimizer_hooks.GradientHardClipping` for scalar array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emcastillo emcastillo emcastillo approved these changes

Assignees

@emcastillo emcastillo

Labels
cat:bug Bug report or fix. 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.

Refactor utility link in optimizer_hooks unit tests
5 participants
@niboshi @emcastillo @chainer-ci @beam2d @asi1024