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

Fix for the case where there are no detections #9784

Merged
merged 1 commit into from Apr 23, 2018

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Feb 13, 2018

Use cv2.cvtColor instead of np to convert from BGR 2 RGB
Fix context in detector

Description

(Brief description on what this PR is about)

Checklist

Essentials

  • Passed code style checking (make lint)
  • 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
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@larroy larroy requested a review from szha as a code owner February 13, 2018 13:50
@larroy
Copy link
Contributor Author

larroy commented Feb 13, 2018

@zhreshold

@@ -43,6 +43,7 @@ class Detector(object):
ctx : mx.ctx
device to use, if None, use mx.cpu() as default context
"""
CLASS = 0
Copy link
Member

Choose a reason for hiding this comment

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

Is this convention used elsewhere? All caps, word 'CLASS', etc?
Is it used somewhere? Can you name it something that isn't a keyword else and not in caps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@larroy larroy Feb 13, 2018

Choose a reason for hiding this comment

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

KLASS? constants should be in caps according to python style (pep8)

Copy link
Member

Choose a reason for hiding this comment

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

"Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL."

This is at class-level, right? Would a derivative not want to override the value?
Is this done elsewhere? I haven't seen thyis done within classes elsewhere, but maybe I'm wrong?

Copy link
Contributor Author

@larroy larroy Feb 13, 2018

Choose a reason for hiding this comment

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

this is at class level, is the equivalent of a static variable (in this case constant) in C++. A variable that is owned by the class itself, instead of the instance. Since it's a constant is in caps. I don't see a problem, other than perhaps the naming. So the variable refers to which class the object belongs to. class is a reserved identifier in python as you mention, and you indicate that having a reserved word just differing by the case might not be good. I don't see a problem, in any case we could rename it to KLASS or CLS as is done usually in python. Maybe @zhreshold has additional comments anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that you mean it for class-level.
Again, question:

"Would a derivative not want to override the value?
Is this done elsewhere (all-caps constant within a class)? I haven't seen this done within classes elsewhere, but maybe I'm wrong?"

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to use cls_index=0 as default argument when instantiating rather than class level static variable

result = []
det = detections[i, :, :]
for obj in det:
if obj[Detector.CLASS] >= 0:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a class somewhere with 'CLASS" defined as something other than zero?

:param detections:
:return:
"""
print(type(detections))
Copy link
Member

Choose a reason for hiding this comment

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

remove print?

:return:
"""
print(type(detections))
assert((type(detections) is mx.nd.NDArray) or (type(detections) is np.ndarray))
Copy link
Member

Choose a reason for hiding this comment

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

use isinstance

Use cv2.cvtColor instead of np to convert from BGR 2 RGB
Fix context in detector
result = []
det = detections[i, :, :]
for obj in det:
if obj[class_idx] >= 0:
Copy link
Member

Choose a reason for hiding this comment

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

How about self.class_idx with default 0 in __init__

Copy link
Contributor Author

@larroy larroy Feb 15, 2018

Choose a reason for hiding this comment

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

not possible, the method is static. That's why there was a class variable on the first place when the review started. Seems we are going in circles.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to use member function rather than static. Is there any specific reason we can't do it? Or it just serves as better self-explained code?
Anyways, I don't bother overcomplicating such a simple class, either way LGTM and I am just trying to settle it down quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filter_positive_detections doesn't need any member data, it's a pure function, reflected by @staticmethod so one knows that is not going to mutate class state. Anyway, maybe is my pedantic defensive programming from other safer programming languages, in Python seems everyone codes however they want.

@cjolivier01
Copy link
Member

cjolivier01 commented Feb 14, 2018

Is there a class somewhere with 'class_idx" defined as something other than zero?

@larroy
Copy link
Contributor Author

larroy commented Feb 15, 2018

@cjolivier01 no, what's your point? its intention is semantic. instead of just using a 0 index which is not clear what it contains.

@larroy
Copy link
Contributor Author

larroy commented Feb 20, 2018

@cjolivier01 any more changes? can we merge this?

@cjolivier01
Copy link
Member

cjolivier01 commented Feb 20, 2018

Where do the other non-zero numbers which index into detections[] come from?

@cjolivier01
Copy link
Member

cjolivier01 commented Feb 20, 2018

What is in the 0th index that makes it special? What guarantees that nothing else will try to use zero?
And if other classes (derivatives, for example) are meant to use other values other than zero, how is overlap to be avoided?

@cjolivier01
Copy link
Member

cjolivier01 commented Feb 20, 2018

If it's just a standin for zero, why is it called class_idx?
Would there every be a static item that's a one? (1)

@larroy
Copy link
Contributor Author

larroy commented Feb 21, 2018

It's the output of the model: class, score, and normalized bounding box coordinates. You could have also the other indices named for better readability. That function only uses the class to filter negative detections which output -1.

@CodingCat
Copy link
Contributor

Hi, the community has passed to vote about associating the code changes with JIRA (https://lists.apache.org/thread.html/ab22cf0e35f1bce2c3bf3bec2bc5b85a9583a3fe7fd56ba1bbade55f@%3Cdev.mxnet.apache.org%3E)

We have updated the guidelines for contributors in https://cwiki.apache.org/confluence/display/MXNET/Development+Process, please ensure that you have created a JIRA at https://issues.apache.org/jira/projects/MXNET/issues/ to describe your work in this pull request and include the JIRA title in your PR as [MXNET-xxxx] your title where MXNET-xxxx is the JIRA id

Thanks!

@anirudh2290
Copy link
Member

@cjolivier01 @zhreshold is this good to merge ?

@zhreshold zhreshold merged commit 81d0113 into apache:master Apr 23, 2018
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
Use cv2.cvtColor instead of np to convert from BGR 2 RGB
Fix context in detector
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
Use cv2.cvtColor instead of np to convert from BGR 2 RGB
Fix context in detector
@larroy larroy deleted the ssd_rework branch November 15, 2018 18:44
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

5 participants