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

Additional fixes for draw_net #5487

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Conversation

nitheeshas
Copy link
Contributor

@nitheeshas nitheeshas commented Apr 4, 2017

The previous fix #5477 for draw_net had some issues

  • draw.py still wasn't working for most of the nets.
  • The unittest didn't get executed:
    • unittest didn't have call for unittest.main() (not sure if there is any way to run unittest without this).
    • import statement for text_format should be from google.protobuf import text_format.

These are fixed in this PR.

@lukeyeager
Copy link
Contributor

unittest didn't have call for unittest.main() (not sure if there is any way to run unittest without this).

Ah, I usually use nose or pytest instead of unittest directly. My bad.

@lukeyeager
Copy link
Contributor

draw.py still wasn't working for most of the nets.

What's the error you're seeing?

The unittest didn't get executed.

Actually, yes it does. Turn on the -v flag to check if you like:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3056d75..d53be93 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -99,7 +99,7 @@ add_custom_target(lint COMMAND ${CMAKE_COMMAND} -P ${PROJECT_SOURCE_DIR}/cmake/l
 
 # ---[ pytest target
 if(BUILD_python)
-  add_custom_target(pytest COMMAND python${python_version} -m unittest discover -s caffe/test WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/python )
+  add_custom_target(pytest COMMAND python${python_version} -m unittest discover -v -s caffe/test WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/python )
   add_dependencies(pytest pycaffe)
 endif()

Copy link
Contributor

@lukeyeager lukeyeager left a comment

Choose a reason for hiding this comment

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

I can't see that any of these changes are required.

separator,
layer.convolution_param.pad[0] if len(layer.convolution_param.pad._values) else 0)
layer.convolution_param.pad[0] if len(layer.convolution_param.pad) else 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think this is necessary?

>>> from caffe.proto import caffe_pb2
>>> net = caffe_pb2.NetParameter()
>>> layer = net.layer.add()
>>> layer.convolution_param.kernel_size
[]
>>> layer.convolution_param.kernel_size.append(1)
>>> layer.convolution_param.kernel_size
[1]
>>> len(layer.convolution_param.kernel_size)
1
>>> len(layer.convolution_param.kernel_size._values)
1

Looks like it should work as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting AttributeError: 'google.protobuf.pyext._message.RepeatedScalarConta' object has no attribute '_values' when trying with the included example nets.

This happens when using protobuf>2.6.1 (NVIDIA#303). I have tested the PR with protobuf==2.5.0 and it works fine, just in case.

@@ -1,7 +1,7 @@
import os
import unittest

from google import protobuf
from google.protobuf import text_format
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I feel like the existing syntax is slightly clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing syntax is more clear, but its returning error:
AttributeError: 'module' object has no attribute 'text_format'

Replacing the import statement as mentioned in PR fixed it. draw_net.py also uses the same syntax, probably because of this issue.



if __name__ == "__main__":
unittest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary when the pytest target is python -m unittest discover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this isn't necessary when running the unittest as you mentioned. My bad!

@lukeyeager
Copy link
Contributor

Aha! So all the fixes are for protobuf >= 3.0. That makes sense. Sounds good to me.

@shelhamer
Copy link
Member

Thanks for the further fixes @nitheeshas !

@shelhamer shelhamer merged commit e9f7941 into BVLC:master Apr 6, 2017
Bardo91 pushed a commit to Bardo91/caffe that referenced this pull request Jun 1, 2017
ethantang95 pushed a commit to ethantang95/caffe that referenced this pull request Jul 25, 2017
, BVLC#5487

from BVLC/caffe

Fix Python net drawing script

Revert "Fix Python net drawing script"

This reverts commit db6cf0a.

Add test for caffe.draw.draw_net()

Minor fix for net drawing script

Add main() for draw_net unittest, fix import errors
ethantang95 pushed a commit to ethantang95/caffe that referenced this pull request Jul 26, 2017
, BVLC#5487

from BVLC/caffe

Fix Python net drawing script

Revert "Fix Python net drawing script"

This reverts commit db6cf0a.

Add test for caffe.draw.draw_net()

Minor fix for net drawing script

Add main() for draw_net unittest, fix import errors
drnikolaev added a commit to NVIDIA/caffe that referenced this pull request Jul 27, 2017
Fix for visualizing caffe models, cherry-picked PRs BVLC#5010, BVLC#5477, BVLC#5487 from BVLC/caffe
drnikolaev added a commit to NVIDIA/caffe that referenced this pull request Jul 27, 2017
Fix for visualizing caffe models, cherry-picked PRs BVLC#5010, BVLC#5477, BVLC#5487 from BVLC/caffe for caffe 0.15
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

3 participants