Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[contrib][op] fix MultiBoxPrior confusing results if first ratio is not 1.0 #13763

Merged
merged 2 commits into from
Apr 18, 2019

Conversation

zhreshold
Copy link
Member

Description

Replace a hardcoded ratio in MultiBoxPrior with whatever user desired.
It was hardcoded to 1 by default, now it allow arbitrary value, even though ratio==1.0 is highly recommended in almost all circumstances for computer vision tasks.

This change does NOT affecting existing examples and use cases.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@Roshrini
Copy link
Member

Roshrini commented Jan 2, 2019

@mxnet-label-bot Add [Operator, pr-awaiting-review]

@marcoabreu marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Jan 2, 2019
@stu1130
Copy link
Contributor

stu1130 commented Jan 16, 2019

@eric-haibin-lin could you review it? Thanks

@sandeep-krishnamurthy
Copy link
Contributor

@zhreshold - Thanks.
Can you please rebase?

@@ -297,6 +297,18 @@ def f(x, a, b, c):
[backward_expected],
rtol=1e-2 if dtype is np.float16 else 1e-5,
atol=1e-2 if dtype is np.float16 else 1e-5)
def test_multibox_prior_op():
h = 561
w = 728
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to not hardcode the test values?

Copy link
Member Author

Choose a reason for hiding this comment

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

the input shape is designed specifically for the network

@ankkhedia
Copy link
Contributor

@eric-haibin-lin ping for review!
@zhreshold Could you look into above comments?

Copy link
Member

@anirudhacharya anirudhacharya left a comment

Choose a reason for hiding this comment

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

@zhreshold can you please explain why we need the changes in the onnx-tensorrt submodule in this PR?

@zhreshold
Copy link
Member Author

Should be fine, I've tried rebasing to master

@karan6181
Copy link
Contributor

@zhreshold Could you please provide an update on this PR? It seems no tracking for last 2 weeks. Thanks !

@zhreshold zhreshold force-pushed the fix-multibox-prior-logic branch 2 times, most recently from edee717 to be55418 Compare April 3, 2019 23:58
@zhreshold zhreshold merged commit 93238a2 into apache:master Apr 18, 2019
@zhreshold zhreshold deleted the fix-multibox-prior-logic branch April 18, 2019 18:21
kedarbellare pushed a commit to kedarbellare/incubator-mxnet that referenced this pull request Apr 20, 2019
…ot 1.0 (apache#13763)

* fix confusion results if ratios are set differently

* update 3rdparty
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
…ot 1.0 (apache#13763)

* fix confusion results if ratios are set differently

* update 3rdparty
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants