Skip to content
This repository was archived by the owner on Jan 3, 2023. It is now read-only.

Sarkars/unify pooling translate functions#420

Open
sayantan-nervana wants to merge 6 commits into
masterfrom
sarkars/unify_pooling_translate_functions
Open

Sarkars/unify pooling translate functions#420
sayantan-nervana wants to merge 6 commits into
masterfrom
sarkars/unify_pooling_translate_functions

Conversation

@sayantan-nervana

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/ngraph_builder.cc
std::shared_ptr<ng::Node> ng_avgpool =
make_shared<ng::op::AvgPool>(ng_input, ng_kernel_shape, ng_strides,
ng_padding_below, ng_padding_above, false);
std::shared_ptr<ng::Node> ng_pool =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question:
Does TF AvgPool considers padding in average calculation? Just want to confirm here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Avg pool has this extra param whose default value is false:
https://github.com/NervanaSystems/ngraph/blob/066037c25901478ac79ead90aa6da721ab27c0c3/src/ngraph/op/avg_pool.hpp#L51

Earlier we were explicitly passing the false value, but even without it, the default value is going to be false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool! Sounds good!

@mingshan-wang mingshan-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@avijit-nervana avijit-nervana force-pushed the sarkars/unify_pooling_translate_functions branch from e696e3d to 7978347 Compare April 12, 2019 16:33
@avijit-nervana avijit-nervana added the Release Candidate PRs needed for the next release label Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Release Candidate PRs needed for the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants