rename field to signal

[andlaus]

this renames the field variables of the encoding and decoding utility functions back to signal and prepares them for piecewise linear functions.

Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information

[andlaus]

as an additional bonus, this PR also adds "inverse choices dicts", which should significantly increase encoding performance for messages which contain many signals that exhibit a lot of choices. @zariiii9003: would be great if you gave this a spin with your DBC file...

[coveralls]

Pull Request Test Coverage Report for Build 4957995570

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 97 of 103 (94.17%) changed or added relevant lines in 8 files are covered.
• 81 unchanged lines in 7 files lost coverage.
• Overall coverage decreased (-0.001%) to 94.089%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cantools/database/can/signal.py	12	13	92.31%
cantools/database/diagnostics/data.py	19	20	95.0%
cantools/database/utils.py	45	46	97.83%
cantools/subparsers/utils.py	3	6	50.0%

Files with Coverage Reduction	New Missed Lines	%
cantools/logreader.py	1	99.46%
cantools/typechecking.py	2	89.29%
cantools/autosar/secoc.py	3	90.63%
cantools/database/can/database.py	6	96.67%
cantools/database/utils.py	12	90.95%
cantools/subparsers/utils.py	19	57.95%
cantools/database/can/message.py	38	90.25%

Totals
Change from base Build 4956348676:	-0.001%
Covered Lines:	7099
Relevant Lines:	7545

---

💛 - Coveralls

[zariiii9003]

as an additional bonus, this PR also adds "inverse choices dicts", which should significantly increase encoding performance for messages which contain many signals that exhibit a lot of choices. @zariiii9003: would be great if you gave this a spin with your DBC file...

Sure. I'm not sure how many choices that message actually has, but decoding with decode_choices=True seems significantly faster. The other measurements are a little slower, probably because of additional function calls. I tested with python 3.11, the function call penalty might be higher with earlier python versions.
Overall, the performance looks fine to me.

With decode_choices

Script:

unpacked = {signal.name: min(0x40, signal.maximum) for signal in message.signals}
packed = bytes([random.randint(0, 255) for _ in range(message.length)])

print(f"{len(message.signals)=}")
n = 10_000
dt = timeit.repeat('data = message.encode(unpacked, strict=False, scaling=True)', globals=globals(), repeat=5, number=n)
print(f"encode time: {min(dt)/n * 1e6} us")
dt = timeit.repeat('message.decode(packed, decode_choices=True, scaling=True)', globals=globals(), repeat=5, number=n)
print(f"decode time: {min(dt)/n * 1e6} us")

master:

len(message.signals)=39
encode time: 12.071810000270489 us
decode time: 18.893299999763258 us

field_to_signal:

len(message.signals)=39
encode time: 13.660729999901378 us
decode time: 7.343199999741046 us

Without decode_choices

Script:

unpacked = {signal.name: min(0x40, signal.maximum) for signal in message.signals}
packed = bytes([random.randint(0, 255) for _ in range(message.length)])

print(f"{len(message.signals)=}")
n = 10_000
dt = timeit.repeat('data = message.encode(unpacked, strict=False, scaling=True)', globals=globals(), repeat=5, number=n)
print(f"encode time: {min(dt)/n * 1e6} us")
dt = timeit.repeat('message.decode(packed, decode_choices=False, scaling=True)', globals=globals(), repeat=5, number=n)
print(f"decode time: {min(dt)/n * 1e6} us")

master:

len(message.signals)=39
encode time: 12.39417000033427 us
decode time: 6.245800000033341 us

field_to_signal:

len(message.signals)=39
encode time: 14.142059999721823 us
decode time: 6.78241000023263 us

Without scaling

Script:

unpacked = {signal.name: min(0x40, signal.maximum) for signal in message.signals}
packed = bytes([random.randint(0, 255) for _ in range(message.length)])

print(f"{len(message.signals)=}")
n = 10_000
dt = timeit.repeat('data = message.encode(unpacked, strict=False, scaling=False)', globals=globals(), repeat=5, number=n)
print(f"encode time: {min(dt)/n * 1e6} us")
dt = timeit.repeat('message.decode(packed, decode_choices=False, scaling=False)', globals=globals(), repeat=5, number=n)
print(f"decode time: {min(dt)/n * 1e6} us")

master:

len(message.signals)=39
encode time: 8.633569999801693 us
decode time: 4.039819999889005 us

field_to_signal:

len(message.signals)=39
encode time: 10.973010000088834 us
decode time: 4.345120000289171 us

[zariiii9003]

isinstance is slow and we shouldn't use assert outside of test code.
I'd suggest return self.choices[cast(int, raw)], because the raw in self.choices check guarantees, that raw is an integer.

[andlaus]

I've pushed a new version of the patch. I'm a bit out of my depths of where the remaining performance regressions stem from. (IMO they are acceptable, though.) Can you help out?

[zariiii9003]

I hope you'll remember all these API changes when you create the next release 😄

[andlaus]

yeah. If not that should not be a too big deal as I'm pretty sure that not too many people use this function as it is pretty low level... (the old function name was a bit of a misnomer IMO, because it also accepted NamedSignalValue objects.)

[zariiii9003]

I think there is something wrong here. If scaling is False, then value must be the raw value. The choice_to_number() was already performed in scaled_to_raw()

[andlaus]

this is pretty twisted:

If scaling is False, then value must be the raw value.

it can also be a choice (i.e., 'str' or 'NamedSignalValue'). it can be argued that considering choices should only be done if scaling is True, thouhgh. (the existing code does it unconditionally, so I thought I keep it this way...)

[zariiii9003]

Maybe we could simplify this too like in the encoding function, if the performance allows it:

if scaling:
    decoded[signal.name] = signal.raw_to_scaled(value, decode_choices)
else:
    decoded[signal.name] = value

[andlaus]

yes, if we decide that choices are only considered if scaling is True and get rid of the decode_choices parameter. (I'm a bit on the line about whether this is a good idea.)

[andlaus]

okay. passing decode_choices to raw_to_scaled simplifies matters a bit: 003efb5 . Thanks for the proposal!

[andlaus]

do you consider this to be ready @zariiii9003?

[zariiii9003]

Data/Signal is also used in utils.create_encode_decode_formats. You could adapt those names, too.

[andlaus]

Data/Signal is also used in utils.create_encode_decode_formats. You could adapt those names, too.

good point. I hope to have captured the non-diagnostic code paths here

[zariiii9003]

I think this is fine. Let's merge this and then i'll have some rebasing or merging to do 😩

[andlaus]

I think this is fine. Let's merge this and then i'll have some rebasing or merging to do weary

I sometimes see this as an opportunity for going over the patch one more time. ;)

Let's get this merged. Thanks, @zariiii9003 !