-
Notifications
You must be signed in to change notification settings - Fork 76
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
Refactor the compu methods #306
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andlaus
force-pushed
the
refactor_compumethods
branch
from
May 22, 2024 16:29
e028fbd
to
0cd2d8c
Compare
kayoub5
reviewed
May 23, 2024
kayoub5
reviewed
May 23, 2024
kayoub5
reviewed
May 23, 2024
kayoub5
reviewed
May 23, 2024
kayoub5
reviewed
May 23, 2024
kayoub5
reviewed
May 23, 2024
now, they work as they are defined by the XSD: all parameters are set by the fields specified by the basic `CompuMethod` class, and the specific behavior of the individual categories are implemented by subclasses. This encompasses extracting the relevant data from their generic form via `__post_init__()` methods in the subclasses and doing the actual computations to translate internal values to/from physical values. This approach has the advantages that it is quite a bit cleaner than the previous code (albeit still quite messy IMO), that all compu methods can be handled using the same output writing code and -- most importantly -- that the code now reflects the specification reasonably closely now. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
andlaus
force-pushed
the
refactor_compumethods
branch
from
May 23, 2024 07:34
0cd2d8c
to
67d2b30
Compare
the XSD is very redundant and messy here: it defines multiple identical groups and does not use inheritance. That said, that does not prevent it to do it better... thanks to [at]kayoub5 for the nudge! Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
…ime data feeding incorrect data to be en-/decoded should only trigger `EncodeError` or `DecodeError`. (`OdxError` means "incorrect ODX model detected") Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
these are reservered for "magic" functions and according to PEP 8 should never be newly created by user code. thanks to [at]kayoub5 for the catch! Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
kayoub5
approved these changes
May 23, 2024
this makes the code conform to section 7.3.6.6.4 of the spec. Note that DOPs using non-invertible SCALE-LINEAR compu methods currently cannot be encoded. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Katja Köhler <[email protected]>
andlaus
force-pushed
the
refactor_compumethods
branch
from
May 23, 2024 12:57
7f7c47a
to
7cf8993
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is quite a whopper (sorry), but all of the changes are closely related to the compu methods and I could not think of a good way of splitting this into smaller chunks.
With this PR, compu methods work as they are defined by the XSD: All parameters of all compu methods are specified using attributes of the basic
CompuMethod
class (i.e., mainlyCOMPU-INTERNAL-TO-PHYS
andCOMPU-PHYS-TO-INTERNAL
) and the specific behavior of the individual categories are implemented in subclasses. "Specific behavior" encompasses extracting the data relevant for the concrete compu method from their generic form via__post_init__()
methods in the subclasses and, based on this, the actual computations to translate internal values to/from physical ones. This approach has the advantages that it is quite a bit cleaner than the previous code (albeit still quite messy IMO), that all compu methods can be handled using the same output writing code and -- most importantly -- that the code now reflects the specification reasonably closely now.Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information