Skip to content

Impl for ParameterList #41259

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
Closed
arktrail wants to merge 1 commit into from
Closed

Impl for ParameterList #41259

arktrail wants to merge 1 commit into from

Conversation

@arktrail
Copy link
Contributor

@arktrail arktrail commented Jul 10, 2020

Summary:
This is a new PR for #40850, #40987 and #41206(I unintentionally closed), as I have some issues for rebates for that one. Very sorry about that. And I have fixed the tests failed in that PR.

This diff contains the implementation of C++ API for ParameterList from #25883.
Refer to the Python API:

pytorch/torch/nn/modules/container.py

Line 376 in bc9e8af

class ParameterList(Module):

Not sure about some naming difference between C++ API and Python API, like append, should it be called push_back

Test Plan: Add unit tests in this diff

Reviewers: @glaringlee

Subscribers:

Tasks: #25883

Tags:

@glaringlee glaringlee requested review from glaringlee and removed request for ebetica and goldsborough July 10, 2020 21:56
@dr-ci
Copy link

dr-ci bot commented Jul 11, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 6e961cf (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 2 times.

@arktrail
Copy link
Contributor Author

arktrail commented Jul 11, 2020

Hi @glaringlee, I have learned how to rebase the PR. Since all the tests are passed, could you please check if this PR is good to go? Thank you very much!

@glaringlee
Copy link
Contributor

glaringlee commented Jul 12, 2020

@yyn19951228
I was waiting all the tests passed. Looks good to me now, will run fb internal test as well, and approve once it passed。

I was wondering whether you could write some in file docs for both parameterList and Dict? It is not necessary for this PR.
like this
https://github.com/pytorch/pytorch/blob/master/torch/csrc/api/include/torch/nn/modules/container/modulelist.h

facebook-github-bot
facebook-github-bot reviewed Jul 12, 2020
View reviewed changes
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.

@glaringlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@arktrail
Copy link
Contributor Author

arktrail commented Jul 12, 2020

@glaringlee
Sure, I will try to do that, and please help polish them as I might have some misunderstanding of some parts.

@glaringlee
Copy link
Contributor

glaringlee commented Jul 13, 2020

@yyn19951228 Thank you so much, and sure, I will help on polishing.

glaringlee
glaringlee approved these changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@glaringlee glaringlee left a comment

Choose a reason for hiding this comment

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

LGTM now

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 13, 2020

@glaringlee merged this pull request in 98df978.

@glaringlee glaringlee mentioned this pull request Oct 23, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@glaringlee glaringlee glaringlee approved these changes

@yf225 yf225 Awaiting requested review from yf225

Assignees

No one assigned

Labels

Merged open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arktrail @glaringlee @facebook-github-bot @pytorchbot @mruberry