Cleaned up image classification cpp example #9799

lebeg · 2018-02-15T18:04:43Z

Description

Cleaned up image classification cpp example.

Checklist

Essentials

Passed code style checking (make lint)
Changes are complete

Changes

Introduced mxnet build option
Enabled build on mac
Cleaned up cmake file
Applied minor code improvements
Applied code formatting

Code improvements

std::string -> const std::string& as method or function parameters
Handling buffer memory with std::unique_ptr<char[]> instead of raw pointer
Use std::size_t instead of int for sizes where possible
Fixed log phrasing and whitespace formatting
Used ostream for logs instead of printf
Used standard exit codes from main
Added license header

Tested

Mac OS
Ubuntu
Windows

larroy · 2018-02-15T18:11:13Z

example/image-classification/predict-cpp/image-classification-predict.cc

+  }
+
+  ~BufferFile() {
+    if (buffer_) {


the check is not needed

Better a smart pointer in any case?

larroy · 2018-02-15T18:12:24Z

example/image-classification/predict-cpp/image-classification-predict.cc

 }

 // LoadSynsets
 // Code from : https://github.com/pertusa/mxnet_predict_cc/blob/master/mxnet_predict.cc
-std::vector<std::string> LoadSynset(std::string synset_file) {
-    std::ifstream fi(synset_file.c_str());
+std::vector <std::string> LoadSynset(std::string synset_file) {


why space before template arg?

larroy · 2018-02-15T18:12:42Z

example/image-classification/predict-cpp/image-classification-predict.cc


-    std::vector<std::string> output;
+  std::vector <std::string> output;


is this your IDE settings?

Thanks, this was a weird default setting on a new machine

piiswrong · 2018-02-15T18:13:07Z

Those keys are release signing keys. If you are not going to drive a release you don't need it

marcoabreu · 2018-02-15T18:34:20Z

CMakeLists.txt

@@ -671,8 +672,7 @@ if(USE_CPP_PACKAGE)
  add_subdirectory(cpp-package)
 endif()

-# Problems on Mac OS X: 1. librt not available 2. mxnet built as MODULE library, which can't be linked.


Has this been resolved?

marcoabreu · 2018-02-15T18:35:11Z

KEYS

@@ -480,3 +480,40 @@ IxR3jVTKye+UerEtN8yATW8CRIKO3IobUfLMDdPCLO7uzoW95cI35Y0l8JgK2NeU
 =E4W5
 -----END PGP PUBLIC KEY BLOCK-----

+pub   rsa2048 2018-02-07 [SC] [expires: 2020-02-07]


Since you're not going to make a release, I don't think that your key should be in here.

marcoabreu · 2018-02-15T18:37:06Z

Could you please add on which platforms you have tested your modifications? Since we're not testing examples on CI, we'll have to test them ourselves.

lebeg

Alright, thanks for letting me know

lebeg · 2018-02-16T10:41:36Z

CMakeLists.txt

@@ -671,8 +672,7 @@ if(USE_CPP_PACKAGE)
  add_subdirectory(cpp-package)
 endif()

-# Problems on Mac OS X: 1. librt not available 2. mxnet built as MODULE library, which can't be linked.


lebeg · 2018-02-16T10:49:35Z

Listed tested platforms in the description

marcoabreu · 2018-02-16T11:05:59Z

Could you please also test Windows?

lebeg · 2018-02-19T16:19:55Z

Added windows to tested platforms

marcoabreu · 2018-02-19T16:23:57Z

example/image-classification/predict-cpp/CMakeLists.txt

+# ---[ OpenCV
+if(NOT USE_OPENCV OR NOT OpenCV_FOUND)
+  message(WARNING "\
+OpenCV should be enabled and found to build image classification example, skipping...")


This is a critical error, right?

The example will just not be build and it will not break the whole project configuration.

This certainly is chopping out a lot of stuff. WHat's the net change in static linking logic?

It is not about static, it's an early return from this cmake file. The example can not be built if there is no OpenCV and will not be added to the main project. That's it.

isn’t the unittest static link flag removed?

UNITTEST_STATIC_LINK is descriptive and editable. Changing the check to MSVC at the actual interpretation point makes it difficult to see what's being done.
Also, I think MSVC has to build static because of dllimport issues

I can leave the variable itself but would rename it, because it's an example, not a unit test.

lebeg · 2018-02-26T13:30:49Z

@marcoabreu @piiswrong @cjolivier01 @szha do you want to take another look and merge if possible?

marcoabreu · 2018-02-26T15:15:16Z

I can't really help here, sorry.

cjolivier01 · 2018-02-26T18:20:16Z

CMakeLists.txt

@@ -32,6 +32,7 @@ mxnet_option(USE_GPROF            "Compile with gprof (profiling) flag" OFF)
 mxnet_option(USE_CXX14_IF_AVAILABLE "Build with C++14 if the compiler supports it" OFF)
 mxnet_option(USE_VTUNE            "Enable use of Intel Amplifier XE (VTune)" OFF) # one could set VTUNE_ROOT for search path
 mxnet_option(ENABLE_CUDA_RTC      "Build with CUDA runtime compilation support" ON)
+mxnet_option(BUILD_CPP_EXAMPLES   "Build cpp examples" OFF)


default to on, please. Otherwise, people may check in/merge broken code.

cjolivier01 · 2018-02-26T18:23:19Z

example/image-classification/predict-cpp/CMakeLists.txt

+# ---[ OpenCV
+if(NOT USE_OPENCV OR NOT OpenCV_FOUND)
+  message(WARNING "\
+OpenCV should be enabled and found to build image classification example, skipping...")


This certainly is chopping out a lot of stuff. WHat's the net change in static linking logic?

cjolivier01 · 2018-02-26T18:24:32Z

example/image-classification/predict-cpp/image-classification-predict.cc

 /*!
- *  Copyright (c) 2015 by Xiao Liu, pertusa, caprice-j


I don't think you can do this, can you? I was under the impression that license text could never be removed.
@smarthi , this this ok?

Technically I'm not removing contributors, just naming them as a group. I followed the pattern in the rest of the project, but will be happy to change it back.

is the guy’s name still there?

Yes, brought it back

cjolivier01 · 2018-02-26T18:25:29Z

example/image-classification/predict-cpp/image-classification-predict.cc


 const mx_float DEFAULT_MEAN = 117.0;

+std::string trim(const std::string& input) {


make static, please

cjolivier01 · 2018-02-26T18:28:09Z

Can you please summarize the "cleaning up" items in the description? It's a little hard to follow what the added value is from the diff, since it's a bit scattered.

lebeg · 2018-02-27T11:05:32Z

@cjolivier01 The changes are already listed:

Changes

Introduced mxnet build option
Enabled build on mac
Cleaned up cmake file
Applied minor code improvements
Applied code formatting

lebeg · 2018-02-27T11:13:57Z

@cjolivier01 please take another look, thanks!

cjolivier01 · 2018-03-01T15:21:32Z

Can you please summarize the "cleaning up" items in the description? It's a little hard to follow what the added value is from the diff, since it's a bit scattered.

cjolivier01 · 2018-03-01T16:33:27Z

Also, is this used in any of the unit tests or nightly tests? If not, probably we should be running at least one test with it.

cjolivier01 · 2018-03-01T16:24:12Z

example/image-classification/predict-cpp/CMakeLists.txt

+# ---[ OpenCV
+if(NOT USE_OPENCV OR NOT OpenCV_FOUND)
+  message(WARNING "\
+OpenCV should be enabled and found to build image classification example, skipping...")


UNITTEST_STATIC_LINK is descriptive and editable. Changing the check to MSVC at the actual interpretation point makes it difficult to see what's being done.
Also, I think MSVC has to build static because of dllimport issues

lebeg · 2018-03-01T16:36:53Z

@cjolivier01 for now this is not tested on ci, but I will ingerate this as the base for valgrind memcheck tests

This reverts commit d5c29d7.

lebeg · 2018-03-01T17:20:06Z

Added 'Code improvements' section for detailisation

* Cleaned up cpp image classification example * Added gpg key * Revert "Added gpg key" This reverts commit d5c29d7. * Adressed review comments, made other small improvements * Reverted default device type to cpu * Minor type change * Applied review comments * Brought back the static linking option

lebeg requested review from cjolivier01 and szha as code owners February 15, 2018 18:04

larroy reviewed Feb 15, 2018

View reviewed changes

marcoabreu reviewed Feb 15, 2018

View reviewed changes

lebeg commented Feb 16, 2018

View reviewed changes

marcoabreu reviewed Feb 19, 2018

View reviewed changes

cjolivier01 suggested changes Feb 26, 2018

View reviewed changes

lebeg force-pushed the cpp-example branch from 2b3b68a to e4b2d27 Compare February 27, 2018 11:12

cjolivier01 suggested changes Mar 1, 2018

View reviewed changes

lebeg added 5 commits March 1, 2018 17:50

Cleaned up cpp image classification example

d627cdc

Added gpg key

59114b9

Revert "Added gpg key"

c1641cf

This reverts commit d5c29d7.

Adressed review comments, made other small improvements

6cc28a0

Reverted default device type to cpu

b2d04e9

lebeg added 3 commits March 1, 2018 17:50

Minor type change

3478b33

Applied review comments

1997156

Brought back the static linking option

4d95bd9

lebeg force-pushed the cpp-example branch from e4b2d27 to 4d95bd9 Compare March 1, 2018 17:20

cjolivier01 approved these changes Mar 1, 2018

View reviewed changes

marcoabreu merged commit 9757091 into apache:master Mar 2, 2018

lebeg deleted the cpp-example branch April 16, 2018 15:22


		std::vector<std::string> output;
		std::vector <std::string> output;


		const mx_float DEFAULT_MEAN = 117.0;

		std::string trim(const std::string& input) {

Cleaned up image classification cpp example #9799

Cleaned up image classification cpp example #9799

Conversation

lebeg commented Feb 15, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Code improvements

Tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piiswrong commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu commented Feb 15, 2018

lebeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeg commented Feb 16, 2018

marcoabreu commented Feb 16, 2018

lebeg commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeg commented Feb 26, 2018

marcoabreu commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjolivier01 commented Feb 26, 2018

lebeg commented Feb 27, 2018

Changes

lebeg commented Feb 27, 2018

cjolivier01 commented Mar 1, 2018

cjolivier01 commented Mar 1, 2018

Choose a reason for hiding this comment

lebeg commented Mar 1, 2018

lebeg commented Mar 1, 2018

lebeg commented Feb 15, 2018 •

edited

Loading