Fix allTrue to return true on empty input vector

[beutlich]

Make Modelica.Math.BooleanVectors.allTrue equivalent to obsolete (and to be removed) function Modelica.StateGraph.Temporary.allTrue.

Closes #682, #2860.

[HansOlsson]

Looks good, but I would also highlight this in the release notes as it is a breaking change.
(Even though the previous behavior was plain weird.)

[beutlich]

Yes, I will care for this kind of "critical" changes. They are usually listed in the release notes as, see for example

ModelicaStandardLibrary/Modelica/package.mo

Lines 2486 to 2487 in b020379

	The following <font color=\"red\"><strong>critical errors</strong></font> have been fixed (i.e., errors
	that can lead to wrong simulation results):

[MartinOtter]

With this change a valid user model might give different results without any notice or error message. Therefore, such a breaking change is critical. I would suggest to only make this change if a conversion script is provided. Having a notice in the release notes is not sufficient. An alternative is to add a second function with a different name.

[beutlich]

I consider this as a bugfix, that will be well documented in the release notes. If we still want to keep/duplicate the current implementation to not break existing behaviour we would also need to duplicate the Modelica.Blocks.MathBoolean.{And, Nand} blocks then. Even worse, behaviour already is inconsistent between Modelica.Math.BooleanVectors.allTrue and Modelica.StateGraph.Temporary.allTrue.

See for example the C++ Standard Library where std::all_of returns true if the range is empty.

[sjoelund]

Well, it would always be possible to add an ObsoleteModelica class or similar with the old, bad functionality, right?

[dietmarw]

Since the current implementation of allTrue is giving an unexpected ("weird") result (even Julia 😏 returns true for empty input) I would propose the following solution.

• have a conversion script to change the use of current Modelica.Math.BooleanVectors.allTrue to ObsoleteModelica4.Modelica.Math.BooleanVectors.allTrue
• change Modelica.Math.BooleanVectors.allTrue itself to the correct behaviour

This means existing models will continue to behave like they did and users making use of the allTrue funtion from MSL will have the correct expected behaviour.

[HansOlsson]

@dietmarw wrote:
...

This means existing models will continue to behave like they did and users making us of the allTrue
funtion from MSL will have the correct expected behaviour.

That is one possibility, and I think it is acceptable and likely the best, the downside is that existing users will have to do more work - for the many cases where the vector isn't empty. Making it conditional on the length of input seems too complicated. Another possibility would be to use convertMessage.

However, assumedly there are not many uses of this function - so a simple safe conversion would do.

[beutlich]

Can't we just fix this corner case as proposed please?

Considering that Modelica.StateGraph.Temporary.allTrue gets converted to Modelica.Math.BooleanVectors.allTrue (since we want the fixed behaviour for backward compatibility), but Modelica.Math.BooleanVectors.allTrue gets converted to ObsoleteModelica4.Math.BooleanVectors.allTrue (to retain the faulty behaviour for backward compatibility), I wonder what happens if conversion will be applied recursively.

Once again, also Modelica.Blocks.MathBoolean.{And, Nand} need to be considered, since those blocks call Modelica.Math.BooleanVectors.allTrue.

I also definitely do not want to introduce a new enumeration just for the backward-compatibility.

type AllTrueIfEmpty = enumeration(FixedBehaviour, DoNotUse_LegacyBehaviour);

[MartinOtter]

I consider this as a bugfix, that will be well documented in the release notes.

This is not a bug-fix but a change of behaviour. The current documentation of the function states:

• Returns true if all elements of the Boolean input vector b are true. Otherwise the function returns false. If b is an empty vector, i.e., size(b,1)=0, the function returns false.

So, the function operates exactly as stated in the documentation and therefore this is clearly no bug fix.

If the Boolean input vector is empty, then there is no input and it is an arbitrary choice what to do in this case. It can be argued that in some applications it gives nicer code if "true" is returned in case of empty vector (but this needs to be anyway analyzed in every particular case), and therefore it would be better to return true in this case.

I understand that conversion scripts are not nice in this case. Therefore, I would propose to keep this function and add a second function with a different name (and use this second function in "nand").

Possible name: andTrue
For symmetry, also a function orTrue could be introduced.

[beutlich]

@MartinOtter If you close a PR with unmerged commits you should also change the milestone to never and add the invalid label. Otherwise the PRs will shos up in the automatically generated release notes.