Signal: __init__: accept initial= as deprecated synonym for raw_initial=

[malsyned]

I have code that creates DBC files from scratch that is broken by the rename of initial= to raw_initial= in be9abad. This PR adds initial= as a synonym for raw_initial which prints a deprecation warning.

[coveralls]

Pull Request Test Coverage Report for Build 5071384531

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.006%) to 93.864%

---

Totals
Change from base Build 5027870252:	0.006%
Covered Lines:	7235
Relevant Lines:	7708

---

💛 - Coveralls

[zariiii9003]

This raises the question, whether initial is a raw or physical value here.

If we do this, then this should be a DeprecationWarning: warnings.warn(deprecation_notice, DeprecationWarning). The deprecation notice should contain the start and end of the deprecation period.

[malsyned]

Agreed about the DeprecationWarning, I'll update the PR to do that with whatever deprecation period you want me to use.

Digging through the history of the usage of this parameter, it looked to me like it always was used to mean the raw value. My main evidence was that In the patch that renamed it, the only change made in dbc.py was:

-                   initial=get_signal_initial_value(frame_id_dbc, signal[1][0]),
+                   raw_initial=get_signal_initial_value(frame_id_dbc, signal[1][0]),

so the semantics of the parameter didn't change, only the name of it.

However, the meaning of Signal.initial did change in be9abad:

-        #: The initial value of the signal, or ``None`` if unavailable.
-        self.initial: Optional[int] = initial
+        #: The initial value of the signal in units of the physical world,
+        #: or ``None`` if unavailable.
+        self.initial: Optional[SignalValueType] = (
+            self.raw_to_scaled(raw_initial) if raw_initial is not None else None
+        )

As we're talking about this, I'm realizing I would want to go even further with this PR. What do you think of this proposal starting from the 38.x behavior:

• rename initial to raw_initial in both Signal.__init__() and Signal.initial
• retain initial consistently as a deprecated synonym for raw_initial in both places for some period of time
• introduce scaled_initial to both SIgnal.__Init__() and as a field of Signal

Put another way, starting from the current head of master:

• rename Signal.initial to Signal.scaled_initial
• introduce scaled_initial= to Signal.__init__() (see malsyned@340d257 for how I'm envisioning that working)
• Make initial= and Signal.initial deprecated synonyms for raw_initial

[andlaus]

Digging through the history of the usage of this parameter, it looked to me like it always was used to mean the raw value.

that's only true for DBC, but not for ARXML. that said, since the other comparable parameters (minimum, maximum) are physical values for all formats, it IMO makes a lot sense to only expose physical values to users, so that they do not normally need to concern themselves with the internal representation of these values and how to convert them...

retain initial consistently as a deprecated synonym for raw_initial in both places for some period of time

before #536 there already was no consistency: ARXML used physical values for everything while everyone else used raw ones for .initial and physical ones for the minima and maxima...

introduce scaled_initial to both SIgnal.Init() and as a field of Signal

If I remember correctly, we decided that adding scaled_ and raw_ prefixes to all relevant values (.minimum, .maximum, .invalid, and .initial) made the code which used these too clunky and decided that all unprefixed values shall be physical quantities from now on (right, @zariiii9003 ?). That said, as you propose in the present PR, I agree that we should introduce a deprecated initial argument as an alias for raw_initial to the constructor of the Signal class to not break people's code too much...

[malsyned]

@andlaus it's a sticky situation to be in, that DBC and ARXML were using this field to mean two different things, I don't envy your predicament as maintainers :) I like the idea of Message and Signal objects having the same semantics regardless of the file type they're read from or written to. I also see how adding scaled_ and raw_ versions of every field would make things unnecessarily clunky without much benefit. Especially when there is raw_to_scaled() and scaled_to_raw() exist. I wouldn't propose it in the general case.

My concern with initial specifically is one of backward compatibility for DBC users (such as myself). In order to meaningfully have that, both the initial= parameter and the Signal.initial field would have to exist and have their previous semantics, and new parameters and fields would have to be added for the new better semantics. Otherwise, the backward compatibility is already broken by the change in semantics for Signal.initial, and I'm better off not having an initial= parameter that seems to work but leads to different behavior.

My interest in a way to specify a scaled initial value in the Signal constructor is that I'm starting to use cantools to create database files from scratch using the Signal, Message, and Database constructors. Is that a use case that you're trying to support with cantools?

I get the feeling I'm jumping into the middle of a conversation without enough context. Do you have any recommended reading for the motivation behind these changes?

[andlaus]

My concern with initial specifically is one of backward compatibility for DBC users (such as myself).

I fully understand. The current battle plan is to make a new major release and to instruct people who use the initial values of signals and non-ARXML formats to pin the version of cantools if they do not want to go over their code. (also, this is going to be clearly stated in the changelog.) I'm aware that we will probably get quite a bit of heat because of this, but I hope it will be manageable, i.e., that not too many people use signal.initial.

Otherwise, the backward compatibility is already broken by the change in semantics for Signal.initial, and I'm better off not having an initial= parameter that seems to work but leads to different behavior.

that's what a new major release means when using semantic versioning. Regardless of what we will do, it will cause some short term pain... (That said, I really want to have this change because before #536

sig_values = { 'foo': msg.get_signal_by_name('foo').initial }
raw_data = msg.encode(sig_values)

was incorrect if you didn't happen to use ARXML as the file format to specify your database.)

My interest in a way to specify a scaled initial value in the Signal constructor is that I'm starting to use cantools to create database files from scratch using the Signal, Message, and Database constructors. Is that a use case that you're trying to support with cantools?

yes, it is a supported use case and that will not change. (I occasionally do the same.) after the next release, instead of passing initial= to the Signal constructor, you need to use raw_initial= and the .initial attribute will be a physical value just like .minimum and .maximum. If you only dump the database object to a file, this will not matter to you, though...

In the medium term, more complicated transformations like piecewise linear functions are in the pipeline, so it is IMO prudent to have this consistent...

I get the feeling I'm jumping into the middle of a conversation without enough context. Do you have any recommended reading for the motivation behind these changes?

the motivation is mainly to remove the conditional footgun above. If you are really interested in how this evolved, you can go through the discussion of #536, but I have to warn you that it has quite a few sidequests...

[malsyned]

I fully understand. The current battle plan is to make a new major release and to instruct people who use the initial values of signals and non-ARXML formats to pin the version of cantools if they do not want to go over their code. (also, this is going to be clearly stated in the changelog.)

In that case, I think I'll withdraw this PR. You've made your peace with breaking the API, so I will too :) Thanks for the work you're doing to fix this inconsistency, both of you.

after the next release, instead of passing initial= to the Signal constructor, you need to use raw_initial= and the .initial attribute will be a physical value just like .minimum and .maximum.

Would you be interested in a different PR that instead made initial= a parameter to the Signal() constructor that took a scaled value, for use in creating databases from scratch in Python? It would be a ValueError to pass both, and whichever was passed would be used to initialize both Signal.initial and Signal.raw_initial.

the motivation is mainly to remove the conditional footgun above. If you are really interested in how this evolved, you can go through the discussion of #536, but I have to warn you that it has quite a few sidequests...

Thanks for the pointer!

[zariiii9003]

@malsyned FYI: there's another breaking change to the Signal API in #563. scale, offset, choices and is_float were removed from Signal.__init__(). Instead, a conversion parameter was added.

[andlaus]

Would you be interested in a different PR that instead made initial= a parameter to the Signal() constructor that took a scaled value, for use in creating databases from scratch in Python? It would be a ValueError to pass both, and whichever was passed would be used to initialize both Signal.initial and Signal.raw_initial.

I think that would be an improvement. In particular because we probably need something analogous for the other quantities (minimum, maximum, and invalid) because depending on what the underlying file format uses to specify them, round-tripping a database could be quite a bit off otherwise: e.g., in DBC if the specified minimum does not align with a value that corresponds exactly to a raw value, the minimum of the signal in the dumped file would deflect by the difference compared to the input file.

another solution to this would be to unconditionally specify all constructor arguments in terms of scaled quantities because -- at least in my current mental model -- scaled quantities can be transformed to raw ones with only minuscule rounding errors but not necessarily the other way round. (we should probably go for "scaled, but with the choices only translated if the underlying file format does so" for these quantities because having to convert named values to their strings before creating the signal object could potentially be quite painful...) do you have any thoughts on that matter, @malsyned and @zariiii9003?

[zariiii9003]

@andlaus for dbc format, the minimum and maximum values are specified as physical values. So to me that seems like added complexity without immediate benefit.