Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

fix print_summary bug and add groups of convolution #9492

Merged
merged 8 commits into from
Feb 19, 2018
Merged

fix print_summary bug and add groups of convolution #9492

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56615e8
fix print_summary bug and add groups of convolution
chinakook Jan 19, 2018
34294b1
Update visualization.py
chinakook Jan 19, 2018
50f0c4c
Update visualization.py
chinakook Jan 22, 2018
ff3a869
Update visualization.py
chinakook Feb 5, 2018
95811c1
Update visualization.py
chinakook Feb 7, 2018
4d36fea
Update visualization.py
chinakook Feb 8, 2018
9bb2573
Update visualization.py
chinakook Feb 17, 2018
52624d4
Update visualization.py
chinakook Feb 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions python/mxnet/visualization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,23 @@ def print_layer_summary(node, out_shape):
pre_filter = pre_filter + int(shape[0])
cur_param = 0
if op == 'Convolution':
if ("no_bias" in node["attrs"]) and int(node["attrs"]["no_bias"]):
cur_param = pre_filter * int(node["attrs"]["num_filter"])
if ("no_bias" in node["attrs"]) and node["attrs"]["no_bias"] == 'True':
num_group = int(node["attrs"]["num_group"]) if \
("num_group" in node["attrs"]) else 1
cur_param = (pre_filter * int(node["attrs"]["num_filter"])) \
// num_group
Copy link
Member

Choose a reason for hiding this comment

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

the outer parentheses are not necessary and make it hard to read. would you clean it up?

for k in _str2tuple(node["attrs"]["kernel"]):
cur_param *= int(k)
else:
cur_param = pre_filter * int(node["attrs"]["num_filter"])
num_group = int(node["attrs"]["num_group"]) if \
("num_group" in node["attrs"]) else 1
cur_param = (pre_filter * int(node["attrs"]["num_filter"])) \
// num_group
for k in _str2tuple(node["attrs"]["kernel"]):
cur_param *= int(k)
cur_param += int(node["attrs"]["num_filter"])
elif op == 'FullyConnected':
if ("no_bias" in node["attrs"]) and int(node["attrs"]["no_bias"]):
if ("no_bias" in node["attrs"]) and node["attrs"]["no_bias"] == 'True':
Copy link
Member

Choose a reason for hiding this comment

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

if bool_str == 'True' -> if bool(bool_str)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this?
bool("False") is True

Copy link
Member

Choose a reason for hiding this comment

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

you're right. @chinakook sorry for the misleading comment.
@piiswrong BTW do you happen to know why we are not using json's built-in boolean type?

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 think i can replace bool(bool_str) with built-in function eval(bool_str).

Copy link
Member

Choose a reason for hiding this comment

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

That would create an injection point, since the files may not be trusted. (e.g. "no_bias": "import shutil; shutil.rmtree('/')")

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 see. Then use

node["attrs"]["no_bias"] == 'True'

back?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member

@szha szha Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The json dump should have made booleans without the parenthesis so that the json's type is used, i.e.

"attrs": {
  "flatten": "True",
  "no_bias": "False",
  "num_hidden": "1000"
}
# should have been
"attrs": {
  "flatten": True,
  "no_bias": False,
  "num_hidden": 1000
}

@piiswrong would it be OK to change json export to properly use json boolean?

Copy link
Contributor

Choose a reason for hiding this comment

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

no. there is compatibility issue

cur_param = pre_filter * (int(node["attrs"]["num_hidden"]))
else:
cur_param = (pre_filter+1) * (int(node["attrs"]["num_hidden"]))
Expand Down