mxnet-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] pracheer commented on issue #8782: Caffe to MXNet code translator
Date Thu, 01 Jan 1970 00:00:00 GMT
pracheer commented on issue #8782: Caffe to MXNet code translator
URL: https://github.com/apache/incubator-mxnet/pull/8782#issuecomment-347362396
 
 
   Overall looks good to me.
   
   Few comments:
   - Some files don't seem to have any license. E.g. [batchnorm.st](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/resources/templates/batchnorm.st).
   - Some sort of guidance/document on how the translator internally works will be useful
since from what I see there are bunch of components for this translator. Such details will
help 1. reviewers better do the review and 2. for anyone else who wants to fix any issues
with the translator later on when you are not around.
   - No unit tests?
   - Do you think it'll be good to exit in case of exceptions like [this](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/Solver.java#L56)
or [this](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/Converter.java#L284)?
Basically fail fast in case of issues? Or would it make more sense to generate a code-comment
which says "// Couldn't convert <blah> lr policy" and continue. Also, with that in mind,
what do you think about having a force flag to the translator which if present ignores such
errors and continues instead of exiting?
   - Probably integrate FAQ with the main README since main README doesn't have too much info?
(this may just be my personal preference though).
   - Out of curiosity what happens when [this](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/Converter.java#L362)
returns null?
   
   Few points mainly around style:
   - Do you think having better class docs would be helpful for anyone trying to maintain
the code?
   - Generally good to have braces for if statements even if it contains only one statement.
Also spaces b/w if and () would be good everywhere. See [google braces guide](https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used)
and [whitespace guide](https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace)
for more. I'm assuming fixing now would require too much effort w/o giving enough in return
so feel free to choose accordingly.
   - I'm seeing some inconsistencies regarding use of final modifier. E.g. [here](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/Converter.java#L48)
and [here](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/CreateModelListener.java#L40).
Generally good to be strict unless we there is a reason to not be.
   - (Minor) Instead of calling this [GenHelper](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/GenHelper.java#L33)
GeneratorHelper may be a more appropriate name since it clearly says what is it about.
   - Also it would be good to expand the imports such as [this](https://github.com/indhub/mxnet/blob/caffe_translator/tools/caffe_translator/src/main/java/io/mxnet/caffetranslator/GenHelper.java#L33).
   
   Feel free to ignore comments in the interest of time/effort if necessary or if you don't
agree with them :)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message