Refactor use of deepcopy by docval #238
Conversation
| @@ -411,7 +411,6 @@ def foo(self, **kwargs): | |||
| rtype = options.pop('rtype', None) | |||
| is_method = options.pop('is_method', True) | |||
| allow_extra = options.pop('allow_extra', False) | |||
| val_copy = __sort_args(_copy.deepcopy(validator)) | |||
There was a problem hiding this comment.
The sorting performed on this line is unnecessary given sorting in lines 429-432 (old line numbers) below. The deepcopy is also unnecessary given the other changes.
Codecov Report
@@ Coverage Diff @@
## dev #238 +/- ##
==========================================
- Coverage 70.48% 70.43% -0.05%
==========================================
Files 32 32
Lines 5933 5923 -10
Branches 1390 1388 -2
==========================================
- Hits 4182 4172 -10
Misses 1320 1320
Partials 431 431
Continue to review full report at Codecov.
|
| @@ -225,7 +224,7 @@ def __parse_args(validator, args, kwargs, enforce_type=True, enforce_shape=True, | |||
| ret[argname] = args[argsi] | |||
| argsi += 1 | |||
| else: | |||
| ret[argname] = arg['default'] | |||
| ret[argname] = _copy.deepcopy(arg['default']) | |||
There was a problem hiding this comment.
This is one of the key changes -- only default values are deep-copied, and only when they are used.
| @@ -436,7 +423,7 @@ def dec(func): | |||
| def func_call(*args, **kwargs): | |||
| self = args[0] | |||
| parsed = __parse_args( | |||
| _copy.deepcopy(loc_val), | |||
| loc_val, | |||
There was a problem hiding this comment.
This is one of the key changes. Previously, this list of dictionaries (the docval specification/validator) was deepcopied on every call to a function with docval.
Fix #237
This PR improves runtime of hdmf tests by about 20% and runtime of pynwb tests by about 25%.
The docval list of dictionaries does not need to be deepcopied on every call to a function with docval. The list of dictionaries consists of just strings, types, tuples of strings and types, and critically here, default values. If the default values are not properly copied when used to set the values of missing arguments, missing arguments will all share the same list/dict default value, which is bad.
Rather than deepcopy the whole list of dictionaries, just deepcopy the default value when it is used to set the value of a missing argument.
This PR also removes a few other unnecessary/redundant calls to
deepcopyregarding docval.