Skip to content

Add cost inference of fwd sparse operators and sparse adagrad #9314

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

Add cost inference of fwd sparse operators and sparse adagrad #9314

jspark1105 wants to merge 1 commit into from

Conversation

@jspark1105
Copy link
Contributor

@jspark1105 jspark1105 commented Jul 10, 2018

Summary: We should also add cost inference for sparse operators in backward pass later.

Differential Revision: D8789240

@jspark1105
Copy link
Contributor Author

jspark1105 commented Jul 10, 2018

@salexspb @mdschatz

salexspb
salexspb reviewed Jul 13, 2018
View reviewed changes
caffe2/operators/segment_reduction_op.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/operators/segment_reduction_op.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

mdschatz
mdschatz reviewed Jul 13, 2018
View reviewed changes
caffe2/core/operator_schema.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@jspark1105
Copy link
Contributor Author

jspark1105 commented Jul 14, 2018

@pytorchbot retest this please

2 similar comments
@pietern
Copy link
Contributor

pietern commented Jul 14, 2018

@pytorchbot retest this please

@jspark1105
Copy link
Contributor Author

jspark1105 commented Jul 15, 2018

@pytorchbot retest this please

@jspark1105
Copy link
Contributor Author

jspark1105 commented Jul 17, 2018 •
edited
Loading

@salexspb and @mdschatz , I hope I addressed all the comments. If so, can we land?

@mdschatz
Copy link
Contributor

mdschatz commented Jul 17, 2018

I'm fine with these changes. Happy to land.

@jspark1105
Copy link
Contributor Author

jspark1105 commented Jul 18, 2018

Cool. Can anyone accept the diff? @mdschatz @salexspb

@bddppq
Copy link
Contributor

bddppq commented Jul 29, 2018

@pytorchbot retest this please

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 2, 2018

@highker

@highker highker self-requested a review August 2, 2018 19:56
highker
highker reviewed Aug 2, 2018
View reviewed changes
caffe2/core/operator_schema.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/sgd/adagrad_op.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/operators/segment_reduction_op.cc Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/operators/segment_reduction_op.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 3, 2018

@pytorchbot retest this please

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 3, 2018

@highker please merge this PR. I don't have a write access :)

@highker
Copy link

highker commented Aug 3, 2018

@jspark1105 me neither.... @orionr could you help? Thanks

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 4, 2018

@orionr @bddppq @Yangqing can anyone help merging this?

facebook-github-bot
facebook-github-bot reviewed Aug 4, 2018
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.

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

@orionr
Copy link
Contributor

orionr commented Aug 4, 2018

@jspark1105 and @highker if you don't have the "Import to Phabricator" button here that means you need to install FaceHub. Do "s facehub" internally and you should find steps. Let me know if you have issues. In the meantime, I've created the diff and we should be able to land it through fbcode and it will then arrive in master.

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 4, 2018

@pytorchbot retest this please

2 similar comments
@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 4, 2018

@pytorchbot retest this please

@jspark1105
Copy link
Contributor Author

jspark1105 commented Aug 5, 2018

@pytorchbot retest this please

@jspark1105 @facebook-github-bot
Add cost inference of fwd sparse operators and sparse adagrad (pytorc…
76dedb0 
…h#9314)

Summary:
We should also add cost inference for sparse operators in backward pass later.
Pull Request resolved: pytorch#9314

Differential Revision: D8789240

fbshipit-source-id: b5b32ab52741b3fb81f6af96777703bd59b24c37
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@jspark1105
Add cost inference of fwd sparse operators and sparse adagrad (pytorc…
b60fddd 
…h#9314)

Summary:
We should also add cost inference for sparse operators in backward pass later.
Pull Request resolved: pytorch#9314

Reviewed By: orionr

Differential Revision: D8789240

Pulled By: jspark1105

fbshipit-source-id: 68c2170f294fe13bcc409276f599b5fa8a98bcd3
@ezyang ezyang added the merged label Jun 26, 2019
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

+3 more reviewers

@salexspb salexspb salexspb left review comments

@mdschatz mdschatz mdschatz left review comments

@highker highker highker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

caffe2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@jspark1105 @pietern @mdschatz @bddppq @highker @orionr @facebook-github-bot @salexspb @ezyang @weiyangfb