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

[MXNET-137]fix parameters name inconsistent for Proposal OP and Multi Proposal OP #10242

Merged

Conversation

wkcn
Copy link
Member

@wkcn wkcn commented Mar 26, 2018

Description

Hi, there.
There is parameters name inconsistent in Proposal OP and Multi Proposal OP.
Proposal OP:
cls_prob vs cls_score

Multi Proposal OP:
cls_prob vs cls_score

It seems that the parameter name should be cls_prob rather than cls_score.
The parameter prompt
Faster R-CNN example

So I replace cls_score with cls_prob, and I change the parameter name in the unittest for Proposal OP and Multi Proposal OP.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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

Changes

  • Replace the parameter name cls_score with cls_prob in Proposal OP and Multi Proposal OP
  • Change the parameter name cls_score in the unit-test for Proposal OP and Multi Proposal OP

@wkcn wkcn requested a review from cjolivier01 as a code owner March 26, 2018 00:13
@piiswrong
Copy link
Contributor

This breaks API compatibility. We can only change this if and when we decide to move these ops out of contrib.
Please put a comment in there as reminder

@wkcn
Copy link
Member Author

wkcn commented Mar 26, 2018

@piiswrong
Thank you!
But I found a problem.
When using mx.nd.contrib.Proposal or mx.nd.contrib.MultiProposal, the parameter name is cls_score.
When using mx.sym.contrib.Proposal or mx.sym.contrib.MultiProposal, the parameter name is cls_prob.

@piiswrong
Copy link
Contributor

that's strange. Are you sure?

@wkcn
Copy link
Member Author

wkcn commented Mar 27, 2018

@piiswrong
Yes. I use the test case, code
If I replace cls_score with cls_prob in the code, it will raise the error:

Traceback (most recent call last):
  File "/home/wkcn/proj/testpy/testmxp.py", line 95, in <module>
    test(rpn_pre_nms_top_n, rpn_post_nms_top_n)
  File "/home/wkcn/proj/testpy/testmxp.py", line 58, in test
    rpn_min_size = rpn_min_size, output_score = True)
  File "<string>", line 82, in Proposal
  File "/home/wkcn/proj/mxnet/python/mxnet/_ctypes/ndarray.py", line 92, in _imperative_invoke
    ctypes.byref(out_stypes)))
  File "/home/wkcn/proj/mxnet/python/mxnet/base.py", line 149, in check_call
    raise MXNetError(py_str(_LIB.MXGetLastError()))
mxnet.base.MXNetError: Cannot find argument 'cls_prob', Possible Arguments:

But it's right to use cls_prob in mx.sym.contrib.Proposal in the code

The reason is that the function ListArguments() in proposal-inl.h returns {"cls_prob", "bbox_pred", "im_info"}, it's used in symbol.
However, .add_argument("cls_score", "NDArray-or-Symbol", "Score of how likely proposal is object.") in proposal.cc is used in ndarray.

@piiswrong
Copy link
Contributor

ok...
I think we should make them consistent by using the sym version

@piiswrong
Copy link
Contributor

@wkcn ping

@wkcn
Copy link
Member Author

wkcn commented Apr 27, 2018

@piiswrong
Hi!
The document shows that the sym version of Proposal OP uses cls_score.
However, the sym version uses cls_prob, and the ndarray version uses cls_score.
My PR has made them consistent to cls_prob.

@WalterMa
Copy link

WalterMa commented May 27, 2018

Same strange issue: symbol and ndarray proposal operator api are inconsistent.
@wkcn Have you try change symbol parameter to cls_score that would not break API compatibility?

@wkcn
Copy link
Member Author

wkcn commented May 27, 2018

@WalterMa
Yes.
If you replace cls_prob with cls_score in this line, the symbol parameter will change to cls_score.

@WalterMa
Copy link

@wkcn
Thanks a lot!

Just personal opinion:
Would it be better to replace cls_prob with cls_score, since in both sym and nd API docs its parameter name is cls_score.
https://mxnet.incubator.apache.org/api/python/ndarray/contrib.html#mxnet.ndarray.contrib.Proposal
https://mxnet.incubator.apache.org/api/python/symbol/contrib.html#mxnet.symbol.contrib.Proposal

But In fact this will also break API compatibility, such as these code in example/rcnn.
Don't know what to do...

This issue has a direct impact on hybrid_forward in gluon that cannot hybridize network used Proposal operator.
Hope it be addressed soon.

@piiswrong piiswrong merged commit 33de266 into apache:master May 29, 2018
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants