Skip to content

Maxpool ceilmode#294

Draft
owulveryck wants to merge 3 commits intomasterfrom
maxpool-ceilmode
Draft

Maxpool ceilmode#294
owulveryck wants to merge 3 commits intomasterfrom
maxpool-ceilmode

Conversation

@owulveryck
Copy link
Copy Markdown
Member

@owulveryck owulveryck commented Jun 7, 2019

TL;DR: This PR is a draft to open the discussion about a possibility to break the API for the Maxpool operator to add a "ceil mode".

Longer:

From version 1.5, onnx supports a "ceil mode" attribute that has an impact on the calculus of the output shape on the Maxpool operator. (see issue #549 and #1113 in the onnx project for more info).

By now, I did not find any model that is using this mode, nevertheless, I've implemented it to do different tests (one again I am only using this branch for testing purpose.)

The problem is that it breaks the API by adding a new attribute to the MaxPool2D function; another option would be to keep this function as-is and silently set the ceilMode to false and to create a new transition function MaxPool2DCeil but it do not really like this idea.

WDYT?

@owulveryck owulveryck requested a review from chewxy June 7, 2019 07:47
@owulveryck
Copy link
Copy Markdown
Member Author

One more thing: this branch also includes in shadow the premises of implementation of dilation

@chewxy
Copy link
Copy Markdown
Member

chewxy commented Jun 9, 2019

I like it. Perhaps we should think of using ...interface{} for MaxPool2D for now.

In the future we would change ...interface{} to ...NNOpt where NNOpt is defined as:

type NNOpt func(Op) error

which would fit into the building of golgi nicely in the future

@zimenglan-sysu-512
Copy link
Copy Markdown

any progress updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants