-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-1083] Add the example to demonstrate the inference workflow using C++ API #13294
[MXNET-1083] Add the example to demonstrate the inference workflow using C++ API #13294
Conversation
@nswamy addressed the latest review comments
|
int num_unit = strtol(hidden_units_string.c_str(), &pNext, 10); | ||
dimensions.push_back(num_unit); | ||
while (*pNext) { | ||
num_unit = strtol(pNext, &pNext, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::stringstream
with >>
operators could be much more readable and safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a little more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::istringstream stream(hidden_units_string);
std::vector<index_t> dimensions;
int num_unit;
while (stream >> num_unit) {
dimensions.push_back(num_unit);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larroy which method would you suggest?
@mxnet-label-bot add [pr-awaiting-review] |
else | ||
echo "FAIL: inception_inference FAILED to identify the image." | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can put all the training and inference unit tests under the unittest folder? and why not use C++ testing framework such as Catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catch is 👍 but gtest is already available in MXNet as a subrepo, so it might be considered as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples are written so that they won't have external dependency. However, will definitely consider gtest for unit tests.
@mxnet-label-bot update [pr-awaiting-response] |
Could you add information about this example in the README file |
@vandanavk Added the ReadMe file for example. |
int num_unit = strtol(hidden_units_string.c_str(), &pNext, 10); | ||
dimensions.push_back(num_unit); | ||
while (*pNext) { | ||
num_unit = strtol(pNext, &pNext, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larroy which method would you suggest?
@mxnet-jenkins The recent CI failure doesn't seem to be related to my changes. Can we re trigger the build? |
Job timed out due to dockerhub failure. Restarted (even if I'm not @mxnet-jenkins). |
Can this PR be merged? I have addressed the review comments. The example is working as expected and demonstrates the usage of C++ API in running inference workflow. |
@mxnet-label-bot update [pr-awaiting-review] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Amol.
I have addressed all the review comments. Can this be merged? Since the purpose of this example is to show the inference workflow, I would like to keep the example simple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Thanks @sandeep-krishnamurthy @lebeg , can you please merge the PR? |
@mxnet-label-bot update [pr-awaiting-merge] |
…ing C++ API (apache#13294) * [MXNET-1083] Add the example to demonstrate the inference workflow using C++ API * [MXNET-1083] Add the example to demonstrate the inference workflow using C++ API * Updated the code to address the review comments. * Added the README file for the folder. * Addressed the review comments * Addressed the review comments to use argmax and default mean values.
@mxnet-label-bot remove [pr-awaiting-merge] |
Description
The PR includes an example that demonstrates the inference workflow using C++ API.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
The example demonstrates how to load the pre-trained model and associated parameter files.
The model and parameter files can be specified as command line arguments.
Comments