Kernel Initializer issue #364

ananno · 2019-07-11T02:29:58Z

To fix the kernel initializer issue in function conv2d in lmnet/lmnet/layers.py.

Motivation and Context

The argument definition of kernel_initializer of the function conv2d in lmnet/lmnet/layers/layers.py has never been passed into tf.layers.conv2d function call resulting it not effective even if it is passed from the calling function. This change is necessary to pass the parameters in tf.layers.conv2d function by defining and passing arguments like kernel_initializer or kernel_regularizer etc.

Description

The kernel_initializer argument is defined in the function argument of conv2d but never passed in tf.layers.conv2d function in the code body. This results to not passing the kernel regularizer defined or passed by the calling function.

The fix is very simple. By removing the argument definition in conv2d function in lmnet/lmnet/layers/layers.py file. The defined argument like kernel_initializer or any others supported by tf.layers.conv2d function can be automatically passed via *args, **kwargs.

Details discussion on this is documented in issue here.

How has this been tested?

From the code it is obvious that the argument definition of kernel_initializer in conv2d function has never been used.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature / Optimization (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

blueoilbot · 2019-07-11T02:30:38Z

Can one of the admins verify this patch?

Joeper214 · 2019-07-17T06:07:29Z

@ananno Please rebase this branch from the current blueoil/master branch.

So that we can only see the commits you did on your branch.
Currently, there are 1,004 commits which many are unrelated to this PR.

ananno · 2019-07-17T10:20:35Z

@ananno Please rebase this branch from the current blueoil/master branch.

So that we can only see the commits you did on your branch.
Currently, there are 1,004 commits which many are unrelated to this PR.

I've resolved the issue. Please check and confirm.
Thank you.

Joeper214 · 2019-07-18T01:41:37Z

run test

iizukak · 2019-07-30T05:33:59Z

@ananno
Thank you for fixing the bug!!
As you comment in this PR, current lmnet implementation looks not pass kernel_initializer to TensorFlow.
So, currently, TensorFlow use default value None
https://www.tensorflow.org/api_docs/python/tf/layers/conv2d

And if your code will be merged, we use xavier_initializer as default (in your screenshot).

I thinks make None as default looks better.
How do you think?

iizukak · 2019-07-30T05:34:44Z

@nlpng
This PR effect many network.
It's good to be merge?

iizukak · 2019-07-30T06:49:04Z

@nlpng and I checked TensorFlow use glorot_uniform when kernel_initializer is None.
And glorot_uniform is Xavier initializer with uniform distribution.
So there is no problem about default initializer.

https://github.com/tensorflow/tensorflow/blob/456fbc0e498e3d10604973de9f46ca48d62267cc/tensorflow/python/keras/layers/convolutional.py#L108

iizukak · 2019-07-31T00:49:27Z

run test

Joeper214 · 2019-08-06T03:01:28Z

@ananno Please fix failed tests as well.

bo-code-review-bot · 2019-12-12T06:02:54Z

This PR needs Approvals as follows.

Ownership Approval for / from iizukak, tkng, ruimashita
Readability Approval for Python from tkng, tsawada

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

Approve all reviews requested to you (readability and ownership) and LGTM review
Approval, LGTM
Approve all ownership reviews
Ownership Approval or OA
Approve all readability reviews
Readability Approval or RA
Approve specified review targets
- Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
- Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python
Approve LGTM review
LGTM

See all trigger comments

Please replace [Target] to review target

Ownership Approval
- Ownership Approval for [Target]
- OA for [Target]
- Ownership Approval
- OA
- Approval
Readability Approval
- Readability Approval for [Target]
- RA for [Target]
- [Target] Readability Approval
- [Target] RA
- Readability Approval
- RA
- Approval
LGTM
- LGTM
- lgtm

ananno · 2019-12-20T08:53:06Z

@tsawada San: Can you please do Readability review on this PR.
Thank you

ananno · 2019-12-20T08:54:47Z

@iizukak San: You approved this PR but I think according to new policy you've to approve the ownership review. Please approve as ownership review.

Thank you

tsawada

Readability Approval

iizukak · 2019-12-20T09:21:21Z

Please check CI status.

lmnet/lmnet/layers/layers.py

iizukak

Ownership Approval

ananno · 2020-01-28T09:30:10Z

@tsawada San:
Can you please review this PR for Readability Approval.

lmnet/lmnet/blocks.py

tkng

Please revert this change, pull request should not include any cosmetic change without reason.

(I meant this line

blueoil/lmnet/lmnet/blocks.py

Line 43 in 984cd16

    
           kernel_initializer=tf.contrib.layers.variance_scaling_initializer,)  # he initializer

)

ananno · 2020-02-03T03:24:39Z

Please revert this change, pull request should not include any cosmetic change without reason.

(I meant this line

blueoil/lmnet/lmnet/blocks.py

Line 43 in 984cd16

kernel_initializer=tf.contrib.layers.variance_scaling_initializer,) # he initializer

)

@tkng San:
Thank you for the explanation. I've revert the cosmetic change. Please review one more time.

tkng · 2020-02-03T04:16:44Z

Readability Approval
Ownership Approval

tkng · 2020-02-03T04:16:50Z

/ready

bo-mergebot · 2020-02-03T04:16:52Z

⏳Merge job is queued...

bo-mergebot · 2020-02-03T04:48:17Z

😥Status check failed. Please fix problems and send /ready again.

Without defining kernel_initializer parameter it will have the default value provided by TF. Also it can be provided via network kwargs.

In darknet the kernel_initializer was wrongly implemented, i.e. function call instead of function name. This bug was hiding under the `kernel_initializer` argument issue in `conv_2D` function in `layers.py`.

ananno · 2020-05-26T09:18:14Z

/ready

ananno · 2020-05-28T06:28:44Z

/ready

bo-mergebot · 2020-05-28T06:28:45Z

⏳Merge job is queued...

hadusam added the CI: auto-run Run CI automatically label Jul 12, 2019

ananno force-pushed the kernel_initializer_issue branch from 88ed213 to 376e1e5 Compare July 17, 2019 10:15

blueoil-butler bot added CI: test-all Run all tests once and removed CI: test-all Run all tests once labels Jul 18, 2019

iizukak self-requested a review July 30, 2019 05:34

iizukak approved these changes Jul 30, 2019

View reviewed changes

blueoil-butler bot added CI: test-all Run all tests once and removed CI: test-all Run all tests once labels Jul 31, 2019

ananno force-pushed the kernel_initializer_issue branch from 693c64d to c83e067 Compare October 3, 2019 05:39

ananno force-pushed the kernel_initializer_issue branch from 0afb811 to 630159b Compare December 20, 2019 08:15

ananno changed the title ~~Kernel Initializer issue~~ WIP: Kernel Initializer issue Dec 20, 2019

ananno force-pushed the kernel_initializer_issue branch from 630159b to 07ffdf4 Compare December 20, 2019 08:49

ananno changed the title ~~WIP: Kernel Initializer issue~~ Kernel Initializer issue Dec 20, 2019

ananno self-assigned this Dec 20, 2019

ananno requested a review from tsawada December 20, 2019 08:52

ananno requested a review from iizukak December 20, 2019 08:54

tsawada reviewed Dec 20, 2019

View reviewed changes

iizukak reviewed Jan 28, 2020

View reviewed changes

lmnet/lmnet/layers/layers.py Outdated Show resolved Hide resolved

ananno requested a review from iizukak January 28, 2020 08:36

iizukak approved these changes Jan 28, 2020

View reviewed changes

tkng reviewed Jan 29, 2020

View reviewed changes

lmnet/lmnet/blocks.py Outdated Show resolved Hide resolved

ananno force-pushed the kernel_initializer_issue branch from 12cca0c to 984cd16 Compare January 29, 2020 07:40

tsawada removed their request for review January 29, 2020 07:41

tkng reviewed Jan 29, 2020

View reviewed changes

ananno force-pushed the kernel_initializer_issue branch from c6c0987 to d5e602b Compare February 3, 2020 03:23

ananno requested a review from tkng February 3, 2020 03:25

Joeper214 added the CI: test-all Run all tests once label Feb 3, 2020

blueoil-butler bot removed the CI: test-all Run all tests once label Feb 3, 2020

tkng added the CI: test-lmnet Run lmnet test once label Feb 3, 2020

blueoil-butler bot removed the CI: test-lmnet Run lmnet test once label Feb 3, 2020

ananno force-pushed the kernel_initializer_issue branch from a575dd5 to c93b5c1 Compare February 5, 2020 08:37

ananno added 2 commits May 26, 2020 17:40

Removing kernel_initializer parameter.

c4b87c9

Without defining kernel_initializer parameter it will have the default value provided by TF. Also it can be provided via network kwargs.

Fix the bug in darknet

d745070

In darknet the kernel_initializer was wrongly implemented, i.e. function call instead of function name. This bug was hiding under the `kernel_initializer` argument issue in `conv_2D` function in `layers.py`.

ananno force-pushed the kernel_initializer_issue branch from 4307767 to d745070 Compare May 26, 2020 08:40

Merge branch 'master' into kernel_initializer_issue

f95feff

bo-mergebot bot merged commit f1e3b43 into blue-oil:master May 28, 2020

ananno deleted the kernel_initializer_issue branch May 28, 2020 06:29

ytfksw mentioned this pull request Jun 1, 2020

tf_upgrade_v2 for blocks.py #895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel Initializer issue #364

Kernel Initializer issue #364

ananno commented Jul 11, 2019 •

edited

Loading

blueoilbot commented Jul 11, 2019

Joeper214 commented Jul 17, 2019 •

edited

Loading

ananno commented Jul 17, 2019

Joeper214 commented Jul 18, 2019

iizukak commented Jul 30, 2019

iizukak commented Jul 30, 2019

iizukak commented Jul 30, 2019 •

edited

Loading

iizukak commented Jul 31, 2019

Joeper214 commented Aug 6, 2019

bo-code-review-bot bot commented Dec 12, 2019

ananno commented Dec 20, 2019

ananno commented Dec 20, 2019

tsawada left a comment

iizukak commented Dec 20, 2019

iizukak left a comment

ananno commented Jan 28, 2020

tkng left a comment •

edited

Loading

ananno commented Feb 3, 2020

tkng commented Feb 3, 2020

tkng commented Feb 3, 2020

bo-mergebot bot commented Feb 3, 2020

bo-mergebot bot commented Feb 3, 2020

ananno commented May 26, 2020

ananno commented May 28, 2020

bo-mergebot bot commented May 28, 2020

Kernel Initializer issue #364

Kernel Initializer issue #364

Conversation

ananno commented Jul 11, 2019 • edited Loading

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

blueoilbot commented Jul 11, 2019

Joeper214 commented Jul 17, 2019 • edited Loading

ananno commented Jul 17, 2019

Joeper214 commented Jul 18, 2019

iizukak commented Jul 30, 2019

iizukak commented Jul 30, 2019

iizukak commented Jul 30, 2019 • edited Loading

iizukak commented Jul 31, 2019

Joeper214 commented Aug 6, 2019

bo-code-review-bot bot commented Dec 12, 2019

ananno commented Dec 20, 2019

ananno commented Dec 20, 2019

tsawada left a comment

Choose a reason for hiding this comment

iizukak commented Dec 20, 2019

iizukak left a comment

Choose a reason for hiding this comment

ananno commented Jan 28, 2020

tkng left a comment • edited Loading

Choose a reason for hiding this comment

ananno commented Feb 3, 2020

tkng commented Feb 3, 2020

tkng commented Feb 3, 2020

bo-mergebot bot commented Feb 3, 2020

bo-mergebot bot commented Feb 3, 2020

ananno commented May 26, 2020

ananno commented May 28, 2020

bo-mergebot bot commented May 28, 2020

ananno commented Jul 11, 2019 •

edited

Loading

Joeper214 commented Jul 17, 2019 •

edited

Loading

iizukak commented Jul 30, 2019 •

edited

Loading

tkng left a comment •

edited

Loading