fix print_summary bug and add groups of convolution #9492
Conversation
1. fix "int(node["attrs"]["no_bias"])" bug 2. add groups of convolution param calculation
python/mxnet/visualization.py
Outdated
| 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 |
There was a problem hiding this comment.
the outer parentheses are not necessary and make it hard to read. would you clean it up?
python/mxnet/visualization.py
Outdated
| 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': |
There was a problem hiding this comment.
if bool_str == 'True' -> if bool(bool_str)
There was a problem hiding this comment.
Are you sure about this?
bool("False") is True
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think i can replace bool(bool_str) with built-in function eval(bool_str).
There was a problem hiding this comment.
That would create an injection point, since the files may not be trusted. (e.g. "no_bias": "import shutil; shutil.rmtree('/')")
There was a problem hiding this comment.
I see. Then use
node["attrs"]["no_bias"] == 'True' back?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
no. there is compatibility issue
|
It' done. |
python/mxnet/visualization.py
Outdated
| 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 |
There was a problem hiding this comment.
int(node['attrs'].get('num_group', '1')). same below.
There was a problem hiding this comment.
dict.get handles default value for missing value so the trailing if is no longer necessary.
|
Wait, in how far does this have test coverage? |
* fix print_summary bug and add groups of convolution 1. fix "int(node["attrs"]["no_bias"])" bug 2. add groups of convolution param calculation * Update visualization.py lint * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py
* fix print_summary bug and add groups of convolution 1. fix "int(node["attrs"]["no_bias"])" bug 2. add groups of convolution param calculation * Update visualization.py lint * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py * Update visualization.py
Description
Checklist
Essentials
make lint)