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] roi_pool operator alter layout #6516

Merged
merged 1 commit into from
Nov 8, 2020
Merged

Conversation

Beya2019
Copy link
Contributor

According to the discussion of roi_align(#6443)

add aoi_pool operator(in fasterrcnn) convert_op_layout and related test case in test_pass_convert_op_layout.py.

Meantime, I also test the case tests/python/frontend/pytorch/test_object_detection.py(changed the generate_jit_model to fasterrcnn) to verify this roi pool operator.

Would you please have a look at this @kevinthesun @anijain2305. Thanks very much.

@Beya2019
Copy link
Contributor Author

Hi @kevinthesun:

can you help to have a look at this submit? Thanks very much.

if (roi_pool_attrs->layout == "NCHW") {
oshape = {rshape[0], dshape[1], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1]};
} else {
CHECK_EQ(roi_pool_attrs->layout, "NHWC") << "Unexpected ROI Pool layout";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add what the expected layout should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tkonolige :
Do you mean this?

  if (roi_pool_attrs->layout == "NCHW") {
    oshape = {rshape[0], dshape[1], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1]};
  } else if (roi_pool_attrs->layout == "NHWC") {
    oshape = {rshape[0], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1], dshape[3]};
  } else {
    LOG(FATAL) << "vision.roi_pool does not support " << roi_pool_attrs->layout << " layout";
  }

@tqchen tqchen changed the base branch from master to main October 11, 2020 18:19
@Beya2019
Copy link
Contributor Author

Hi @yzhliu, can you help to have a look at this submit? Thanks very much.

Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

otherwise good to me.

python/tvm/relay/op/vision/_rcnn.py Show resolved Hide resolved
Copy link
Contributor Author

@Beya2019 Beya2019 left a comment

Choose a reason for hiding this comment

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

otherwise good to me.
Hi @yzhliu :

Very fhanks for your review, I coded like this for reference conv2d and conv3d:

    # Handle default kernel layouts
    if desired_data_layout == "NCHW":
        new_attrs["kernel_layout"] = "OIHW"
        return relay.nn.conv2d(data, weight, **new_attrs)
    elif desired_data_layout == "NHWC":
      ....
        return relay.nn.conv2d(data, weight, **new_attrs)
    raise ValueError("Layout %s is not yet supported." % desired_data_layout)

original layout will not go to this process(raising exception)
original layout and Legal desired layout will go the if case which covers the all supported layout ["NCHW", "NHWC"](As shown below), Only if user gives a not supported layout, the raise ValueError case can report the exception.

    if desired_data_layout in ["NCHW", "NHWC"]:
        return relay.vision.roi_pool(data, rois, **new_attrs)

    raise ValueError("Layout %s is not yet supported." % desired_data_layout)

if (roi_pool_attrs->layout == "NCHW") {
oshape = {rshape[0], dshape[1], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1]};
} else {
CHECK_EQ(roi_pool_attrs->layout, "NHWC") << "Unexpected ROI Pool layout";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tkonolige :
Do you mean this?

  if (roi_pool_attrs->layout == "NCHW") {
    oshape = {rshape[0], dshape[1], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1]};
  } else if (roi_pool_attrs->layout == "NHWC") {
    oshape = {rshape[0], roi_pool_attrs->pooled_size[0], roi_pool_attrs->pooled_size[1], dshape[3]};
  } else {
    LOG(FATAL) << "vision.roi_pool does not support " << roi_pool_attrs->layout << " layout";
  }

Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

my bad, was thinking of other stuffs.

@Beya2019
Copy link
Contributor Author

my bad, was thinking of other stuffs.

Can you help merge the pull request directly? Thanks very much

@tqchen tqchen merged commit 82f3e4b into apache:main Nov 8, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
Co-authored-by: honghua.cao <honghua.cao@streamcomputing.com>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
Co-authored-by: honghua.cao <honghua.cao@streamcomputing.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
Co-authored-by: honghua.cao <honghua.cao@streamcomputing.com>
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.

5 participants