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

Draft of formula data record #302

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

n135c10r
Copy link
Collaborator

Description

Proposition for FormulaDataRecord

@n135c10r n135c10r added the implementation Changes to code label Jan 15, 2025
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 3.89610% with 74 lines in your changes missing coverage. Please review.

Project coverage is 97.50%. Comparing base (a2ee482) to head (4afc9b4).

Files with missing lines Patch % Lines
uds/database/data_record/formula_data_record.py 0.00% 74 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #302      +/-   ##
===========================================
- Coverage   100.00%   97.50%   -2.50%     
===========================================
  Files           49       50       +1     
  Lines         2890     2966      +76     
  Branches       356      358       +2     
===========================================
+ Hits          2890     2892       +2     
- Misses           0       74      +74     
Flag Coverage Δ
integration-tests 78.52% <3.89%> (-2.00%) ⬇️
integration-tests-branch 73.73% <3.89%> (-1.87%) ⬇️
unit-tests 97.50% <3.89%> (-2.50%) ⬇️
unit-tests-branch 97.50% <3.89%> (-2.50%) ⬇️
Files with missing lines Coverage Δ
uds/database/data_record/abstract_data_record.py 100.00% <100.00%> (ø)
uds/database/data_record/formula_data_record.py 0.00% <0.00%> (ø)

def contains(self) -> Tuple["AbstractDataRecord", ...]:
return ()

def decode(self, raw_value: int) -> float:
Copy link
Owner

Choose a reason for hiding this comment

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

please adjust the code to handle following scenario:
raw_value | physical_value
0 | 0 km/h (decoding/encoding using encode/decode function)
1 | 0.1 km/h (decoding/encoding using encode/decode function)
...
2999 | 299.9 km/h (decoding/encoding using encode/decode function)
3000 | 3000 (no decoding/encoding - as per RawDataRecord; potentially to be handled by TableDataRecord encoding/decoding and adding labels as physical values like Fault, Init)
3001 | 3001 (no decoding/encoding - as per RawDataRecord; potentially to be handled by TableDataRecord encoding/decoding and adding labels as physical values like Fault, Init)
...

In other words we need:

  • range where (endocidng/decoding) formulas are applied
  • option to mix it with other DataRecords outside the formula range

raw_value = super().decode(raw_value)
return self._decode_function(raw_value)

def encode(self, physical_value: float) -> int:
Copy link
Owner

Choose a reason for hiding this comment

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

same remark as per decode


def decode(self, raw_value: int) -> DecodedDataRecord:
decoded_data_record: DecodedDataRecord = super().decode(raw_value)
physical_value = (decoded_data_record.raw_value / self._factor) + self._offset
Copy link
Owner

@mdabrowski1990 mdabrowski1990 Jan 16, 2025

Choose a reason for hiding this comment

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

I think it would be easier to just create encode_formula and decode_formula in init using factor and offset. Then you can inherit everything after CustomFormulaDataRecord cause all the rest would be the same.

Something like that (simplified):

class LinearFormulaDataRecord(CustomFormulaDataRecord):
   def __init__(..., factor, offset, ...):
       super(..., encode_fomula=lambda x: (x-offset)/factor, decode_formula=lambda x: x*factor + offset, ...

)
self.decode_formula = decode_formula
self.encode_formula = encode_formula
self.formula_range = formula_range
Copy link
Owner

Choose a reason for hiding this comment

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

if formula_range argument is None, then we should set full range (0, (1<<length)-1) here..

"""
Encode raw value for provided physical value.

:param physical_value: Physical (meaningful e.g. float, str type) value of this Data Record.

:return: Raw Value of this Data Record.
"""
if not isinstance(physical_value, int):
# TODO: after changing typing for physical_value this method need to be adjusted
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need change here? I thought this part is already good.

@@ -102,17 +102,19 @@ def decode(self, raw_value: int) -> DecodedDataRecord:
)
return DecodedDataRecord(name=self.name, raw_value=raw_value, physical_value=raw_value)

def encode(self, physical_value: DataRecordPhysicalValueAlias) -> int:
def encode(self, physical_value: DataRecordPhysicalValueAlias) -> DataRecordPhysicalValueAlias:
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we want to change output?

Copy link
Owner

@mdabrowski1990 mdabrowski1990 left a comment

Choose a reason for hiding this comment

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

I am not sure why you changed RawDataRecord. The rest looks really good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Changes to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants