Skip to content
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.

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

[Relay][Op] Adaptive pooling #3085

Merged
merged 13 commits into from
May 9, 2019
Merged

Conversation

kevinthesun
Copy link
Contributor

Adaptive pooling operator.

@@ -332,6 +332,22 @@ struct GlobalPool2DAttrs : public tvm::AttrsNode<GlobalPool2DAttrs> {
}
};

/*! \brief Attributes for global pool operator */
Copy link
Member

Choose a reason for hiding this comment

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

global -> adaptive?

Copy link
Contributor

@markrogersjr markrogersjr 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

layout="NCHW"):
r"""2D adaptive max pooling operator.

This operator takes data as input and does 2D average value calculation
Copy link
Contributor

Choose a reason for hiding this comment

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

average->max

auto i_end_h = end_index(output[height_axis], out_height, height);
auto i_start_w = start_index(output[width_axis], out_width, width);
auto i_end_w = end_index(output[width_axis], out_width, width);
auto dheight = tvm::reduce_axis(Range(0, i_end_h - i_start_h), "rv1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is naming the IterVar rv1 necessary?

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 is optional.

const Expr& odim,
const Expr& idim) {
Expr tmp = (out_index + 1) * idim / odim;
return tvm::ir::Select::make((out_index + 1) * idim % odim == 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to explicitly take the ceil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ceil will involve several casts and causing result inaccurate.


Parameters
----------
outs: Array of Tensor
The computation graph description of global_pool
The computation graph description of adaptive_poo
Copy link
Contributor

Choose a reason for hiding this comment

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

adaptive_poo->adaptive_pool

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits.

const auto* data = types[0].as<TensorTypeNode>();
if (data == nullptr) { return false; }
const auto dshape = data->shape;
CHECK_NE(dshape.size(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Remove? since dshape.size() >= 2

CHECK_NE(dshape.size(), 0);
CHECK_GE(dshape.size(), 2U)
<< "Pool2D only support input >= 2-D: input must have height and width";
const auto param = attrs.as<AdaptivePool2DAttrs>();
Copy link
Member

Choose a reason for hiding this comment

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

const auto* to reduce confusion since as returns a pointer?

@tqchen
Copy link
Member

tqchen commented Apr 27, 2019

Please take some time to discuss the API naming and docs, as adaptive_pooling seems to be something that is not conventional.

@kevinthesun
Copy link
Contributor Author

kevinthesun commented Apr 28, 2019

@tqchen This naming comes from pytorch pooling family: https://pytorch.org/docs/stable/_modules/torch/nn/modules/pooling.html MXNet has a similar API for adaptive avg pooing: https://mxnet.apache.org/api/python/ndarray/contrib.html?highlight=adap#mxnet.ndarray.contrib.AdaptiveAvgPooling2D For now I don't see a more appropriate name in mainsteam DL frameworks. Also adaptive describes the behavior of this kind of pooling.

@tqchen
Copy link
Member

tqchen commented Apr 28, 2019

let us make it contrib.adaptive_pooling then and mark as experimental

@kevinthesun
Copy link
Contributor Author

@tqchen This is ready to be reviewed.

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

Could you also fix the mxnet converter for adaptive pooling?

topi/python/topi/cuda/pooling.py Outdated Show resolved Hide resolved
topi/python/topi/x86/pooling.py Outdated Show resolved Hide resolved
@icemelon
Copy link
Member

icemelon commented May 7, 2019

@kevinthesun Could you also update the mxnet converter for adaptive pooling?

@kevinthesun
Copy link
Contributor Author

@kevinthesun Could you also update the mxnet converter for adaptive pooling?

Already updated.

@icemelon
Copy link
Member

icemelon commented May 7, 2019

Sorry, didn't notice that...

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

Notice Tianqi's comment. Could you change the nn.contrib_adaptive_avg_pool2d and nn.contrib_adaptive_max_pool2d to contrib.adaptive_avg_pool2d and contrib.adaptive_avg_pool2d?

Please also update the doc at docs/langref/relay_op.rst and docs/api/python/topi.rst.

@tqchen
Copy link
Member

tqchen commented May 8, 2019

I will leave the PR management to @icemelon9

@icemelon icemelon merged commit 147ea3b into apache:master May 9, 2019
@icemelon
Copy link
Member

icemelon commented May 9, 2019

Thanks everyone. This is now merged.

wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
* Add topi adaptive_pool

* Use adaptive_pool to compute global_pool

* Add relay adaptive pool2d

* Fix lint

* Fix typo

* Minor change

* Change support level to 10

* Add contrib

* Remove global pool schedule

* Add contrib module

* Fix lint

* Update doc

* Update doc
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
* Add topi adaptive_pool

* Use adaptive_pool to compute global_pool

* Add relay adaptive pool2d

* Fix lint

* Fix typo

* Minor change

* Change support level to 10

* Add contrib

* Remove global pool schedule

* Add contrib module

* Fix lint

* Update doc

* Update doc
@kevinthesun kevinthesun deleted the AdaptivePooling branch May 28, 2019 23:19
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.

6 participants