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

[filter/tvm] Fix dimension order and add custom prop #3342

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

kimjh12
Copy link
Contributor

@kimjh12 kimjh12 commented Jun 15, 2021

Bug fix: inversed dimension order
Add custom prop num_input_tensors

Signed-off-by: Junhwan Kim jejudo.kim@samsung.com

Self evaluation:

  1. Build test: [ ]Passed [ ]Failed [*]Skipped
  2. Run test: [ ]Passed [ ]Failed [*]Skipped

@taos-ci
Copy link
Collaborator

taos-ci commented Jun 15, 2021

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #3342. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://nnstreamer.mooo.com/.

Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@kimjh12, 💯 All CI checkers are successfully verified. Thanks.

@kimjh12 kimjh12 force-pushed the tvm-fix branch 2 times, most recently from 1a26e8f to 7564a4f Compare June 15, 2021 04:38
Copy link
Member

@anyj0527 anyj0527 left a comment

Choose a reason for hiding this comment

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

LGTM ✔

Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@kimjh12, 💯 All CI checkers are successfully verified. Thanks.

@@ -162,7 +161,15 @@ tvm_subplugin::parse_custom_prop (const char *custom_prop)
nns_loge ("Unknown device (%s).", option[1]);
invalid_option = true;
}
} else {
} else if (g_ascii_strcasecmp (option[0], "num_input") == 0) {
inputInfo.num_tensors = MIN ( atoi (option[1]), NNS_TENSOR_SIZE_LIMIT );
Copy link
Collaborator

Choose a reason for hiding this comment

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

prevent security report later

recommend to use g_ascii_strtoull() (atoi() is obsolescent function)
see https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

(guint) g_ascii_strtoull (option[1], NULL, 10);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!! Thanks for the info

@myungjoo
Copy link
Member

num_inputs looks ambiguous for nnstreamer users although it's not for TVM users.
num_input_tensors would be better for both backgrounds.

For TVM community, you may suggest (and implement) new API or new options for "get num inputs" API.

Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@kimjh12, 💯 All CI checkers are successfully verified. Thanks.

Bug fix: inversed dimension order
Add custom prop `num_input_tensors`
- Since `get_num_inputs` API for tvm model returns (#inputs + #params), user should specify only #inputs

Signed-off-by: Junhwan Kim <jejudo.kim@samsung.com>
Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@kimjh12, 💯 All CI checkers are successfully verified. Thanks.

@kimjh12
Copy link
Contributor Author

kimjh12 commented Jun 16, 2021

num_inputs looks ambiguous for nnstreamer users although it's not for TVM users.
num_input_tensors would be better for both backgrounds.

For TVM community, you may suggest (and implement) new API or new options for "get num inputs" API.

Sounds great. (FYI : https://discuss.tvm.apache.org/t/10238)

@myungjoo myungjoo merged commit 2fc2d10 into nnstreamer:main Jun 17, 2021
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.

None yet

5 participants