Fix performance of reading trace parameters#59
Fix performance of reading trace parameters#59matthijsgeers-keysight merged 2 commits intomasterfrom
Conversation
| if not type(self)._has_expected_type(value): | ||
| raise TypeError(f'A {type(self).__name__} must have a value of type "{type(self)._expected_type_string}"' | ||
| f', but it has a type of {type(value)}') | ||
| def __init__(self, value, skip_validation=False): |
There was a problem hiding this comment.
The main change is here. I added an additional parameter skip_validation that by default is set to False, but that is set to True in the deserialization methods.
There was a problem hiding this comment.
The rest of the changes are whitespace changes because we don't have a formatter set up in this project.
|
I agree with the proposed fix, but there's a lot of refactoring noise in the commit. I would prefer if the code changes were split up into a non-functional code style changes commit and a commit that solves the issue described in #58, as this would allow for an easy revert down the line if it ever turns out to be necessary |
There's a check that we do to ensure that when constructing a TraceParameter (or derived) object, then the elements in the arrays of trace parameters all have the same type. While this may be beneficial when constructing such objects in code, for example when writing traces, it has zero benefit when reading traces, as the type of the elements is simply assumed.
rwols
force-pushed
the
fix-read-performance
branch
from
0198f18 to
9ebe3b6
Compare
| name = read_parameter_name(raw) | ||
| value = TraceSetParameter.deserialize(raw) | ||
| result[name] = value | ||
| StringKeyOrderedDict.__setitem__(result, name, value) |
There was a problem hiding this comment.
Why were these updated to their less readable counterparts? That doesn't seem idiomatic to me
There was a problem hiding this comment.
The class TraceParameterMap inherits from StringKeyOrderedDict.
TraceParameterMap overrides the __setitem__ method of StringKeyOrderedDict. In the overridden method, additional type checking is performed.
This type checking is not needed when deserializing, so we call the base class' method.
2 participants
There's a check that we do to ensure that when constructing a TraceParameter (or derived) object, then the elements in the arrays of trace parameters all have the same type.
While this may be beneficial when constructing such objects in code, for example when writing traces, it has zero benefit when reading traces, as the type of the elements is simply assumed.
close #58