Skip to content

Export ReduceMean/ReduceFrontMean/ReduceBackMean (Caffe2) to ReduceMean (ONNX). #16727

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

Export ReduceMean/ReduceFrontMean/ReduceBackMean (Caffe2) to ReduceMean (ONNX). #16727

wants to merge 1 commit into from

Conversation

@xkszltl
Copy link
Contributor

@xkszltl xkszltl commented Feb 4, 2019 •
edited
Loading

The second input (lengths) is not supported.

@xkszltl xkszltl mentioned this pull request Feb 4, 2019
Closed
@xkszltl
Copy link
Contributor Author

xkszltl commented Feb 5, 2019

Test added.
CI failure looks unrelated.

@xkszltl xkszltl force-pushed the mean branch 2 times, most recently from b58ca42 to b23b50c Compare February 10, 2019 16:52
@ezyang ezyang requested a review from yinghai February 10, 2019 20:51
@ezyang
Copy link
Contributor

ezyang commented Feb 10, 2019

@yinghai could you please review this?

@yinghai yinghai requested a review from houseroad February 11, 2019 00:58
houseroad
houseroad reviewed Feb 11, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

ReduceFrontMean and ReduceBackMean are deprecated now. Could you (also) add support for ReduceMean operator (https://github.com/pytorch/pytorch/blob/master/caffe2/operators/reduce_ops.cc#L221)?

@xkszltl xkszltl changed the title Export ReduceFrontMean/ReduceBackMean (Caffe2) to ReduceMean (ONNX). Export ReduceMean/ReduceFrontMean/ReduceBackMean (Caffe2) to ReduceMean (ONNX). Feb 11, 2019
@xkszltl
Copy link
Contributor Author

xkszltl commented Feb 11, 2019

@houseroad Added

@xkszltl xkszltl force-pushed the mean branch 2 times, most recently from 69a4618 to bd59779 Compare February 11, 2019 16:32
houseroad
houseroad reviewed Feb 11, 2019
View reviewed changes
caffe2/onnx/onnx_exporter.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

using additional block to avoid dup names seems uncommon in PyTorch codebase. Could you remove this trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This provides more benefit than what you thought.

  1. Use it_suffix is unnecessary and it makes code becomes longer.
  2. With const we get SSA, which is easier to read for "local" var.
  3. With {} I can get even more, i.e. terminate the life span earlier before reassignment. This narrows the "local" semantics and helps people navigating in the code.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't see the benefits here...

Especially, the logic here is so simple, and you can just reuse it name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine...Don't wanna drag this PR longer.
IMHO reuse is bad, and makes the first occurrence asymmetric to the rest.

houseroad
houseroad reviewed Feb 12, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Please make sure the coding style is consistent with the whole file.

caffe2/onnx/onnx_exporter.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Also, the extra block is useless here. And it will make the coding style so inconsistent...

@xkszltl
Copy link
Contributor Author

xkszltl commented Feb 12, 2019

Error unrelated.
CI tests failed to start.

DOCKER_IMAGE: 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-cuda10-cudnn7-py3-gcc7:285
Error response from daemon: manifest for 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-cuda10-cudnn7-py3-gcc7:285 not found
Exited with code 1

houseroad
houseroad approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad 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. Thanks.

facebook-github-bot
facebook-github-bot reviewed Feb 12, 2019
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.

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

@xkszltl xkszltl deleted the mean branch February 12, 2019 22:24
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

@houseroad houseroad houseroad approved these changes

@yinghai yinghai Awaiting requested review from yinghai

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xkszltl @ezyang @facebook-github-bot @houseroad