Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Conversion Classes #563

Merged
merged 19 commits into from
May 16, 2023
Merged

Conversation

zariiii9003
Copy link
Collaborator

Followup to the discussion in #537
The tests pass and i don't see any performance regression yet.

@andlaus @1atabey1 This is still in draft stage, but i'd like to hear your opinion.

Open points:

  • scale, offset, choices and is_float exist in both Signal and BaseConversion classes. At the very least, the Signal attributes should be converted to properties, which point to the attributes of Signal.conversion
  • add comments where necessary
  • documentation update

@coveralls
Copy link

coveralls commented May 12, 2023

Pull Request Test Coverage Report for Build 4985412034

  • 237 of 270 (87.78%) changed or added relevant lines in 14 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 93.852%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cantools/database/namedsignalvalue.py 4 5 80.0%
cantools/database/utils.py 22 23 95.65%
cantools/database/can/signal.py 28 34 82.35%
cantools/database/diagnostics/data.py 22 34 64.71%
cantools/database/conversion.py 111 124 89.52%
Totals Coverage Status
Change from base Build 4982762184: -0.2%
Covered Lines: 7221
Relevant Lines: 7694

💛 - Coveralls


return LinearConversion(scale=scale, offset=offset, is_float=is_float)

return NamedSignalConversion(scale=scale, offset=offset, choices=choices)
Copy link
Member

Choose a reason for hiding this comment

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

hm. we possibly might need an NamedSignalIntegerConversion because it is possible to define scaling factor of 1 and an integer offset with some named values (alternatively, all conversion classes could either return integers or floats)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NamedSignalConversion calls the factory internally, so it always uses the correct conversion anyway

Copy link
Member

@andlaus andlaus left a comment

Choose a reason for hiding this comment

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

I support this and would like to get it merged as soon as it is ready...

@zariiii9003 zariiii9003 marked this pull request as ready for review May 13, 2023 23:14
@zariiii9003 zariiii9003 marked this pull request as draft May 14, 2023 02:25
@zariiii9003
Copy link
Collaborator Author

Here's the performance with of this PR :

signal_count=39, signals_with_choices=36
encode scaled numeric: 12.786000006599352 us
encode scaled choices: 12.166000014985912 us
encode raw numeric: 7.9149999874061905 us
encode raw choices: 11.801000000559725 us
decode, no scaling, no choices: 4.096000011486467 us
decode, scaling, no choices: 8.483999990858138 us
decode, no scaling, choices: 16.614000014669728 us
decode, scaling, choices: 9.739999986777548 us
decode, no scaling, no choices, allow_truncated: 1.910999999381602 us

Master branch:

signal_count=39, signals_with_choices=36
encode scaled numeric: 10.641999979270622 us
encode scaled choices: 22.46300002298085 us
encode raw numeric: 7.559000005130656 us
encode raw choices: 21.396000011009164 us
decode, no scaling, no choices: 3.9209999886224978 us
decode, scaling, no choices: 5.720000008295756 us
decode, no scaling, choices: 15.386000013677405 us
decode, scaling, choices: 17.01699999102857 us
decode, no scaling, no choices, allow_truncated: 3.9780000224709515 us

andlaus/piecewise_linear;

signal_count=39, signals_with_choices=36
encode scaled numeric: 22.53899998322595 us
encode scaled choices: 25.100000020756852 us
encode raw numeric: 10.096999976667576 us
encode raw choices: 22.768999988329597 us
decode, no scaling, no choices: 3.9979999928618786 us
decode, scaling, no choices: 11.583999985305127 us
decode, no scaling, choices: 5.404000003181864 us
decode, scaling, choices: 10.812000000441913 us
decode, no scaling, no choices, allow_truncated: 4.25300000642892 us

Measured with python 3.11 on a i7-11850H

@zariiii9003 zariiii9003 marked this pull request as ready for review May 14, 2023 16:46
@andlaus
Copy link
Member

andlaus commented May 15, 2023

Here's the performance with of this PR [...]

interesting. I suppose the much better encoding performance is due to "inverse choice dictionaries"? the few performance regressions in your results are quite acceptable IMO...

@zariiii9003
Copy link
Collaborator Author

@andlaus rebase is done

Copy link
Contributor

@1atabey1 1atabey1 left a comment

Choose a reason for hiding this comment

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

I'm a big fan of this change and would also like to see it merged soon! It certainly makes the implementation of my piecewise linear conversion significantly simpler, thanks!

Copy link
Member

@andlaus andlaus left a comment

Choose a reason for hiding this comment

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

LGTM

@zariiii9003 zariiii9003 merged commit 1ca1775 into cantools:master May 16, 2023
13 checks passed
@zariiii9003 zariiii9003 deleted the conversion_class branch May 16, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants