Skip to content
This repository has been archived by the owner on Jul 2, 2021. It is now read-only.

Add Conv2DActiv #384

Merged
merged 4 commits into from
Aug 8, 2017
Merged

Add Conv2DActiv #384

merged 4 commits into from
Aug 8, 2017

Conversation

yuyu2172
Copy link
Member

@yuyu2172 yuyu2172 commented Aug 8, 2017

Split from #265.

@yuyu2172 yuyu2172 added this to the v0.7 milestone Aug 8, 2017

>>> l = Conv2DActiv(5, 10, 3)

2. Omit :obj:`in_channels` or fill it with :obj:`None`:
Copy link
Member

Choose a reason for hiding this comment

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

The order of descriptions should be same as that of examples.
I mean, Fill :obj:in_channels with :obj:None: or omit it: is better. (Changing the order of examples is also OK)

@@ -1,3 +1,5 @@
from chainercv.links.connection.conv_2d_activ import Conv2DActiv
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 block instead of connection? connection sounds too ambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it to be consistent with the Chainer's directory structure.
connection seems to include links with learnable parameters (e.g. Convolution2D) and a block of them (e.g. Inception).
If we change the directory structure, we need to come up with a new name for group of links that are not blocks, but categorized under connection.

Copy link
Member

Choose a reason for hiding this comment

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

I made it to be consistent with the Chainer's directory structure.

I see. connection is OK.

{'args_style': 'None'},
{'args_style': 'omit'}
)
class TestConv2DActivForward(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Can you merge TestConv2DActivForward and TestConv2DActivBarckward into one class?

@testing.parameterize(
{'args_style': 'explicit'},
{'args_style': 'None'},
{'args_style': 'omit'}
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 this ?

 'args_style': ['explicit', 'None', 'omit'],
 'activ: [None, _add_one]

@yuyu2172
Copy link
Member Author

yuyu2172 commented Aug 8, 2017

I reflected your comments.

Copy link
Member

@Hakuyume Hakuyume left a comment

Choose a reason for hiding this comment

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

LGTM

@Hakuyume Hakuyume merged commit 4b28fa5 into chainer:master Aug 8, 2017
@yuyu2172 yuyu2172 deleted the conv2d-active branch August 8, 2017 11:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants