Skip to content

Conversation

@yl-to
Copy link
Contributor

@yl-to yl-to commented Jul 2, 2020

fix #40227
Removed the sorting operation both in ModuleDict class, updated the docstring.
Also remove a sort operation in corresponding unit test, which will lead to unit test fail.

BC Note: Python version after 3.6, the plain dict will preserve the order of keys.
example:
For a python 3.6+ user, if he is initial a ModuleDict instance using plain python dict:
{
"b": torch.nn.MaxPool2d(3),
"a": torch.nn.MaxPool2d(3)
}
, he will get a ModuleDict which preserve the order:
ModuleDict(
(b): MaxPool2d(kernel_size=3, stride=3, padding=0, dilation=1, ceil_mode=False)
(a): MaxPool2d(kernel_size=3, stride=3, padding=0, dilation=1, ceil_mode=False)
)

For a python 3.5 user, if we maintain the same input, then the output ModuleDict could be:
ModuleDict(
(a): MaxPool2d(kernel_size=3, stride=3, padding=0, dilation=1, ceil_mode=False)
(b): MaxPool2d(kernel_size=3, stride=3, padding=0, dilation=1, ceil_mode=False)
)

@yl-to yl-to requested a review from apaszke as a code owner July 2, 2020 09:25
@yl-to
Copy link
Contributor Author

yl-to commented Jul 2, 2020

@albanD @colesbury

Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

All tests are green now. Perfect !

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@albanD is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yl-to
Copy link
Contributor Author

yl-to commented Jul 2, 2020

All tests are green now. Perfect !

@albanD Hi, is there anything I need to do now?

@albanD
Copy link
Collaborator

albanD commented Jul 2, 2020

No, I am landing this.
The PR will be closed and the bot will comment with the commit hash corresponding to the master merge.

@yl-to
Copy link
Contributor Author

yl-to commented Jul 3, 2020

No, I am landing this.
The PR will be closed and the bot will comment with the commit hash corresponding to the master merge.

Hi, is there any blockage? It is still not merged. @albanD

@yl-to
Copy link
Contributor Author

yl-to commented Jul 6, 2020

No, I am landing this.
The PR will be closed and the bot will comment with the commit hash corresponding to the master merge.

Hi, is there any blockage? It is still not merged. @albanD

Hi @albanD , can you check this PR when you are free, thanks!

@albanD
Copy link
Collaborator

albanD commented Jul 6, 2020

No blockage. It just took a while to get all the internal tests to pass and I wanted to avoid landing over the long week end.

@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in 54d7a1e.

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.

ModuleDict documentation misleadingly describes ordering of keys

5 participants