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

Add assignment support for Sequential #4931

Merged
merged 5 commits into from Feb 7, 2018
Merged

Conversation

Stonesjtu
Copy link
Contributor

Simply a copy from ModuleList.
I think the Sequential is just a forwardable version of ModuleList, it makes sense to combine both into one class.

@@ -62,6 +60,9 @@ def __getitem__(self, idx):
next(it)
return next(it)

def __setitem__(self, idx, module):
return setattr(self, str(idx), module)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Simply manipulate the variable returned by iterator only refers to a new
module object, rather than modify the existing one.
# Specify the module by index
model[1] = nn.Sigmoid()
# Specify the module by name if initialized with OrderedDict
model.relu1 = nn.LeakyReLU()

This comment was marked as off-topic.

def _get_item_by_idx(self, iterator, idx):
"""Get the idx-th item of the iterator"""
size = len(self)
if not (-size <= idx < size):

This comment was marked as off-topic.

for i in range(idx):
next(it)
return next(it)
return self._get_item_by_idx(self._modules.items(), idx)

This comment was marked as off-topic.

Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Looks good now, but I forgot to ask for tests for this feature. Can you please add them?

@Stonesjtu
Copy link
Contributor Author

How do you think of using pure python list rather than OrderedDict for named initialization in Sequential. e.g.

items = list(args)
for item in items:
    if isinstance(item, tuple):
        ...
    else:
        ...

@apaszke
Copy link
Contributor

apaszke commented Feb 6, 2018

@pytorchbot test this please

@apaszke
Copy link
Contributor

apaszke commented Feb 6, 2018

No, let's not complicate the API. This looks good as is. Thanks!

@apaszke apaszke merged commit f796080 into pytorch:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants