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

Add NonMaxSupression op to contribution ops #60

Merged
merged 8 commits into from
Dec 1, 2018
Merged

Conversation

HectorSVC
Copy link
Contributor

No description provided.

@HectorSVC HectorSVC requested a review from a team as a code owner November 29, 2018 19:08
wenbingl
wenbingl previously approved these changes Nov 29, 2018
bool SuppressByIOU(const T* boxes_data, int32_t box_index1, int32_t box_index2) const;
void MaxMin(const T& lhs, const T& rhs, T& min, T& max) const;

private : int64_t max_output_size_;
Copy link
Member

@wenbingl wenbingl Nov 29, 2018

Choose a reason for hiding this comment

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

a new line? #Resolved

"Integer representing the maximum number of boxes to be selected by non max suppression.",
AttributeProto::INT)
.Attr(
"iou_threshold",
Copy link
Member

@wenbingl wenbingl Nov 29, 2018

Choose a reason for hiding this comment

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

any default value for it? #Resolved

@HectorSVC HectorSVC requested review from ke1337 and a team November 29, 2018 23:47

if (max_output_size_ <= 0 || boxes_dims[0] == 0) {
std::vector<int64_t> output_dims(1, 0);
TensorShape output_shape(output_dims);
Copy link
Contributor

@tracysh tracysh Nov 29, 2018

Choose a reason for hiding this comment

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

TensorShape can take an initializer list, so this can be output_shape({1, 0}) to save the overhead of a local vector alloc/destroy. Same thing below for {1, num_to_copy}. #Resolved

Copy link
Contributor

@tracysh tracysh Nov 30, 2018

Choose a reason for hiding this comment

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

Well, not exactly what I said above, whatever the transform from what you have to an initializer list. #Resolved

sorted_scores_with_index.pop();

bool selected = true;
// Check with existing boxes, suppress if exceed the IOU (Intersection Over Union) threadhold
Copy link
Contributor

@tracysh tracysh Nov 30, 2018

Choose a reason for hiding this comment

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

threadhold->threshold? #Resolved

Copy link
Contributor Author

@HectorSVC HectorSVC Nov 30, 2018

Choose a reason for hiding this comment

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

good catch. 👍 #Resolved

.Attr(
"score_threshold",
"Float tensor representing the threshold for deciding when to remove boxes based on score.",
AttributeProto::FLOAT);
}
Copy link
Member

@duli2012 duli2012 Nov 29, 2018

Choose a reason for hiding this comment

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

Do we want to add shape inference here since the shape inference is enabled by default now? #Resolved

@HectorSVC HectorSVC merged commit 31780ca into master Dec 1, 2018
@HectorSVC HectorSVC deleted the non_max_supression branch December 1, 2018 01:15
souptc pushed a commit that referenced this pull request Dec 17, 2018
…from execution

- Remove construction of MLValue name -> idx mapping from execution
  frame to inference session since it needs to be done per session only.
- Minor change in Executor interface (eliminate one heap allocation).

Related work items: #60
tmccrmck pushed a commit to tmccrmck/onnxruntime that referenced this pull request Aug 28, 2019
…data

Missing test data from the rename change
natke referenced this pull request in natke/onnxruntime Feb 15, 2022
* Address compliance issue and some minor updates

* minor update

* Add poli exclusion for RESNETLABELMAP.CS
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.

5 participants