Implemented and tested MultiAnd / MultiOr #3915

AHaumer · 2021-12-13T16:29:32Z

Comparing more than 2 Boolean signals with logical blocks {and, or} quickly gets tedious.
For these cases I've implemented Modelica.Blocks.Logical.{MultiAnd, MulitOr},
using Modelica.Math.BooleanVectors.allTrue and Modelica.Math.BooleanVectors.anyTrue.

HansOlsson · 2021-12-14T08:55:51Z

Modelica/Blocks/Logical.mo

@@ -21,7 +21,10 @@ the output is <strong>false</strong>.
  end And;

  block MultiAnd "Logical and of Boolean vector elements"
-    extends Modelica.Blocks.Interfaces.PartialBooleanMISO;


Why are you switching from connector with automatic sizing to vector input without that feature?
To me that just seems like worse usability.

If you want another name for the input that's fine - but it can still have automatic sizing.

(Oh, and how do you comment on an entire commit - not on specific lines?)

@HansOlsson: Well, try to connect two arrays to the connector with automatic sizing:
BooleanPulse b1[m];
BooleanPulse b2[m];
MultiOr multiOr;
connect(b1.y, multiOr.u);
connect(b2.y, multiOr.u);
"The connection to a connector with connectorSizing parameter must have literal size."
With the second proposal no problem:
BooleanPulse b1[m];
BooleanPulse b2[m];
MultiOr multiOr(nin=2*m);
connect(b1.y, multiOr.u[1:m]);
connect(b2.y, multiOr.u[m+1:2*m);

It seems there are two possibilities how to implement this and I agree with @HansOlsson that it is indeed confusing to combine them both. Esp. if the classes' naming suggests the same handling.

To me this suggests that we could improve the GUI for connectorSizing (both in tools and in the non-normative part of the specification).
Note that the second proposal written in textual form should still work with connectorSizing.

No it doesn't (I tried it).

Well, it is called nu but the following works in Dymola (Dymola 2021x and current):

package T block MultiOr "Logical or of Boolean vector elements" extends Modelica.Blocks.Interfaces.PartialBooleanMISO; equation y=Modelica.Math.BooleanVectors.anyTrue(u); annotation (Icon(coordinateSystem(initialScale=0.1), graphics={ Text(extent={{-90,40},{90,-40}},textString="or")}), Documentation(info="<html> The output is true if any of the elements of the input vector are true, otherwise the output is false. </html>")); end MultiOr; model TestMoel parameter Integer m=4; Modelica.Blocks.Sources.BooleanPulse booleanPulse[m](period=1:m) annotation (Placement(transformation(extent={{-120,32},{-100,52}}))); Modelica.Blocks.Sources.BooleanPulse booleanPulse1[m](period=1:m) annotation (Placement(transformation(extent={{-112,-22},{-92,-2}}))); MultiOr multiOr(nu=2*m) annotation (Placement(transformation(extent={{-28,8},{-8,28}}))); equation connect(booleanPulse.y, multiOr.u[1:m]) annotation (Line(points={{-99,42},{-34,42}, {-34,18},{-28,18}}, color={255,0,255})); connect(booleanPulse1.y, multiOr.u[m+1:2*m]) annotation (Line(points={{-91,-12}, {-34,-12},{-34,18},{-28,18}}, color={255,0,255})); annotation (Icon(coordinateSystem(preserveAspectRatio=false)), Diagram( coordinateSystem(preserveAspectRatio=false))); end TestMoel; annotation (uses(Modelica(version="4.0.0"))); end T;

(Yes, it would work even with typos corrected.)

henrikt-ma · 2021-12-14T09:43:40Z

Related to this, but not to be handled in this PR, isn't it a mistake that allTrue and anyTrue are implemented using an algorithm with iteration rather than using min and max array reductions?

AHaumer · 2021-12-14T21:35:07Z

@henrikt-ma: To be honest, I don't know. Maybe because OM has troubles with vectorized function calls: see Modelica.Electrical.PolyPhase.Basic.{Star, Delta} and some more.
Feel free to propose a PR. - Thanks for the PR.
Also strange: allTrue and andTrue are (nearly) the same.

Modelica/Blocks/Logical.mo

henrikt-ma · 2021-12-14T23:40:38Z

@henrikt-ma: To be honest, I don't know. Feel free to propose a PR.

OK, I'll do it for the sake of demonstrating the power of the language.

Also strange: allTrue and andTrue are (nearly) the same.

Good point, I turned that into a suggestion.

henrikt-ma · 2021-12-15T06:25:25Z

When introducing MultiAnd and MultiOr, I would also have expected a MultiNand, a MultiNor and a MultiXor. The latter should be based on oneTrue, and yes, the name MultiXor might be a bit controversial, but then OneTrue could be a less controversial name. (For completeness, one could also imagine having an OddTrue corresponding to the alternative definition of variadic exclusive disjunction.)

henrikt-ma

I think others are better suited than me for approving or requesting changes on MSL PRs – I'll try to stick to just leaving comments. Right now, I am satisfied with the use of the weirdly named andTrue instead of allTrue, and my only remaining remark is the concern about only introducing variadic variants of the And and Or blocks, while leaving Xor, Nand and Nor without variadic counterparts.

sjoelund · 2021-12-15T07:45:08Z

Also strange: allTrue and andTrue are (nearly) the same.

The normal would be to have allTrue(arr) be min(arr). But for some reason it wasn't. It's actually allTrueAndArrayNotEmpty(arr)... So there is a second function there. It's really confusing :)

Maybe because OM has troubles with vectorized function calls: see Modelica.Electrical.PolyPkase.Basic.{Star, Delta} and some more.

I don't see any vectorized calls in those. Also, reductions are not vectorized calls. min(arr) is not treated as a reduction in OpenModelica (it simply has its own implementation). Vectorized calls in OpenModelica are transformed into reduction expressions: f(arr) = {f(i) for i in arr}.

HansOlsson

To me the lack of connectorSizing is problematic.
If it were the only And-block I could sort of understand it as you normally would connect two inputs and it would be good that it was pre-configured, but we have another variant for that so the Multi-variants don't need to be focused on that.
Note that there are two relevant use-cases covered by connector sizing:

Just connecting a number of scalar signals
Connecting an entire vector input to the model to this input on a sub-component

In both cases both adjusting nin and connecting seems unnecessary, and would be handled automatically by connectorSizing. If there is some reason not to that should at least be documented.
Note that the vector to vector-case indicate that nin=0 and nin=1 are relevant.

AHaumer · 2021-12-15T09:47:33Z

@HansOlsson don't forget the case when you want to connect more than one vector.
I feel somehow uncomfortabel with "solve that with the GUI". Any sound solution?

AHaumer · 2021-12-15T09:49:12Z

@henrikt-ma I have to admit I was too busy to consider Nand, Nor, and Xor. And yes you are right, MultiXor is somehow confusing. How would you define it?
And we shouldn't forget MultiNot.

HansOlsson · 2021-12-15T09:52:14Z

@henrikt-ma I have to admit I was too busy to consider Nand, Nor, and Xor. And yes you are right, MultiXor is somehow confusing. How would you define it? And we shouldn't forget MultiNot.

I think we shouldn't add MultiXor yet because there are multiple possibilities (parity seems more common) and we don't have users demanding it yet.
I don't see a need for MultiNot - the normal Not work, and if you want to negate a vector you just make a vector of Nots

henrikt-ma · 2021-12-15T09:54:18Z

@henrikt-ma I have to admit I was too busy to consider Nand, Nor, and Xor. And yes you are right, MultiXor is somehow confusing. How would you define it?

For me, defining it in terms of oneTrue would be the natural choice, but because of the risk of confusion, it might be better to instead make two variants (neither of them called MultiXor): OneTrue and OddTrue

And we shouldn't forget MultiNot.

Yes, let's forget MultiNot.

HansOlsson · 2021-12-15T10:04:41Z

Regarding multi-xor: https://electronics.stackexchange.com/questions/93713/how-is-an-xor-with-more-than-2-inputs-supposed-to-work
But as stated before I don't think we should add it until needed.

AHaumer · 2021-12-15T16:31:05Z

@HansOlsson you're right, MultiNot is not necessary.
And I'd avoid MultiXor - for me it's extermely confusing ;-)
Ok we should add MultiNand and MulitNor after clarifying the input (vector of automaticSizing).

tobolar · 2021-12-15T16:39:42Z

don't forget the case when you want to connect more than one vector.

@AHaumer Would in that case Modelica.Blocks.Routing.Multiplex2 work?

AHaumer · 2021-12-15T16:52:50Z

@tobolar I'll try that - I think that'll work, you're right!
Not it doesn't work since we don't have a BooleanMulitplex2 ...
And no, I'm not going to implement the BooleanMultiplex{2,3,4,...}

HansOlsson · 2021-12-15T17:00:29Z

@HansOlsson don't forget the case when you want to connect more than one vector. I feel somehow uncomfortabel with "solve that with the GUI". Any sound solution?

To me there are two cases, and I see neither as a blocker: either you write textually and then connectorSizing is a non-issue, or you use the GUI and then it's a matter of improving the GUIs to handle this.

AHaumer · 2021-12-15T17:45:24Z

@HansOlsson sigh ... ok let's do it that way you suggest.
Although it's not very intuitive for newbies until the GUI is improved.

henrikt-ma · 2021-12-15T19:21:26Z

Regarding multi-xor: https://electronics.stackexchange.com/questions/93713/how-is-an-xor-with-more-than-2-inputs-supposed-to-work But as stated before I don't think we should add it until needed.

Right, all the evidence suggests that at least the name MultiXor should be avoided. Better to have both OneTrue and OddTrue, and in my opinion it would make sense to define them along with MultiAnd and MultiOr so that the MSL provides one canonical block for performing each of the operations.

Modelica/Resources/Reference/Modelica/Blocks/Examples/DemoMultiAndOr/comparisonSignals.txt

HansOlsson

seems good, except for the comment about missing reference signals that aren't that critical.

christiankral · 2021-12-22T13:15:37Z

Modelica/Blocks/package.mo

+<li>and2.y vs. multiAnd.y</li>
+<li>notA.y vs. multiNand.y</li>
+<li>or2.y  vs. multiOr.y</li>
+<li>notB.y vs. multiNor.y</li>


I am not sure of compare a vs b is the correct terminology. This is how I understand it:

Compare a vs b means to me that a is on the vertical axis, b is on the horizontal axis

Compare a with b means to me that a and b are plotted as a function of time (in the same graph)

Compare a and b means to me the same as 2.

I am kindly asking for a native speaker to comment on this

Co-authored-by: Henrik Tidefelt <[email protected]>

henrikt-ma · 2024-01-22T07:47:23Z

Modelica/Blocks/Logical.mo

 <p>
-The output is <strong>true</strong> if exactly one input is <strong>true</strong>, otherwise
+The output is <strong>true</strong> if at least one input is <strong>false</strong>, otherwise


This could be formulated more consistently with MultiOr (since there is always a non-zero number of inputs):

Suggested change

The output is true if at least one input is false, otherwise

The output is true if any of the inputs is false, otherwise

I don't see that there has to be at non-zero number of inputs.

We have u1 and u2, which adds up to two inputs, no more, no less.

Modelica/Blocks/Logical.mo

henrikt-ma · 2024-01-22T07:57:33Z

Modelica/Blocks/Logical.mo

+<p>
+The output is <strong>true</strong> if all elements of the input vector are <strong>true</strong>, otherwise
+the output is <strong>false</strong>.
+</p>


There should be a warning about not being the negation of MultiNand:

Suggested change



Note that the result is not the negation of MultiAnd in the case of an empty input vector.

Modelica/Blocks/Logical.mo

henrikt-ma · 2024-01-22T07:59:20Z

Modelica/Blocks/Logical.mo

+<p>
+The output is <strong>true</strong> if at least one element of the input vector is <strong>false</strong>, otherwise
+the output is <strong>false</strong>.
+</p>


There should be a warning about not being the negation of MultiAnd:

Suggested change



Note that the result is not the negation of MultiNand in the case of an empty input vector.

henrikt-ma · 2024-01-22T08:05:34Z

Modelica/Blocks/Logical.mo

    extends Blocks.Interfaces.partialBooleanSI2SO;
  equation
-    y = not (u1 and u2);
+    y = not ((u1 and u2) or (not u1 and not u2));


Wouldn't it be easier expressed like this?

Suggested change

y = not ((u1 and u2) or (not u1 and not u2));

y = (u1 and not u2) or (not u1 and u2);

henrikt-ma

The documentation needs to be written more carefully, specially due to the weird semantics of allTrue.

HansOlsson · 2024-01-22T08:12:46Z

The documentation needs to be written more carefully, specially due to the weird semantics of allTrue.

But it has been changed to use andTrue instead as far as I can tell.

henrikt-ma · 2024-01-22T10:41:00Z

The documentation needs to be written more carefully, specially due to the weird semantics of allTrue.

But it has been changed to use andTrue instead as far as I can tell.

No, allTrue is used in MultiAnd.

HansOlsson · 2024-01-22T10:41:04Z

Modelica/Blocks/Logical.mo

+  block MultiAnd "Logical and of Boolean vector elements"
+    extends Modelica.Blocks.Interfaces.PartialBooleanMISO;
+  equation
+    y = Modelica.Math.BooleanVectors.andTrue(u);
+    annotation (Icon(coordinateSystem(initialScale=0.1),
+      graphics={ Text(extent={{-90,40},{90,-40}},textString="and")}),
+      Documentation(info="<html>
+<p>
+The output is <strong>true</strong> if all elements of the input vector are <strong>true</strong>, otherwise


Suggested change

block MultiAnd "Logical and of Boolean vector elements"

extends Modelica.Blocks.Interfaces.PartialBooleanMISO;

equation

y = Modelica.Math.BooleanVectors.andTrue(u);

annotation (Icon(coordinateSystem(initialScale=0.1),

graphics={ Text(extent={{-90,40},{90,-40}},textString="and")}),

Documentation(info="<html>



The output is true if all elements of the input vector are true, otherwise

block MultiAnd "Logical and of Boolean vector elements"

extends Modelica.Blocks.Interfaces.PartialBooleanMISO;

equation

y = Modelica.Math.BooleanVectors.andTrue(u);

annotation (Icon(coordinateSystem(initialScale=0.1),

graphics={ Text(extent={{-90,40},{90,-40}},textString="and")}),

Documentation(info="<html>



The output is true if all elements of the input vector are true, otherwise

Here is the current code of the PR. Where does it use allTrue?

I'm sorry, my bad. It is MultiNand that still uses allTrue:

block MultiNand "Logical nand of Boolean vector elements" extends Modelica.Blocks.Interfaces.PartialBooleanMISO; equation y=not Modelica.Math.BooleanVectors.allTrue(u);

So, shouldn't we fix that so that Nand is the same as Not And?

I'd love to see that kind of change, but can we do it this late in the release cycle considering that it is a bugfix on the border to a plain backwards incompatible change?

I'd love to see that kind of change, but can we do it this late in the release cycle considering that it is a bugfix on the border to a plain backwards incompatible change?

The models are new so we don't have to care about backwards compatibility, and to me it looks like MultiAnd was corrected, but not MultiNand.

@AHaumer, could you please consider fixing MultiNand as part of this PR, so that we don't have to handle the annoying difference between allTrue and andTrue in the documentation?

henrikt-ma · 2024-01-22T10:54:08Z

Modelica/Blocks/Logical.mo

+            textColor={0,0,0})}),
+      Documentation(info="<html>
+<p>
+The output is <strong>true</strong> if at least one element of the input vector is <strong>false</strong>, otherwise


This is where we need to deal with the use of allTrue:

Suggested change

The output is true if at least one element of the input vector is false, otherwise

The output is true if the input vector is empty or at least one element of the input vector is false, otherwise

HansOlsson · 2024-01-22T12:35:27Z

Modelica/Blocks/Logical.mo

+  block MultiNand "Logical nand of Boolean vector elements"
+    extends Modelica.Blocks.Interfaces.PartialBooleanMISO;
+  equation
+    y=not Modelica.Math.BooleanVectors.allTrue(u);


Suggested change

y=not Modelica.Math.BooleanVectors.allTrue(u);

y=not Modelica.Math.BooleanVectors.andTrue(u);

Change this as well to use andTrue - so that Nand=Not-And.

@AHaumer, could you please consider including this change?

AHaumer added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks labels Dec 13, 2021

AHaumer requested review from christiankral, MartinOtter, tobolar, beutlich and GallLeo December 13, 2021 16:29

AHaumer self-assigned this Dec 13, 2021

AHaumer requested a review from dietmarw December 13, 2021 16:31

beutlich removed their request for review December 13, 2021 21:35

HansOlsson reviewed Dec 14, 2021

View reviewed changes

AHaumer requested a review from HansOlsson December 14, 2021 21:42

henrikt-ma reviewed Dec 14, 2021

View reviewed changes

Modelica/Blocks/Logical.mo Outdated Show resolved Hide resolved

henrikt-ma mentioned this pull request Dec 15, 2021

Use boolean array reductions #3919

Merged

AHaumer requested a review from henrikt-ma December 15, 2021 06:55

henrikt-ma reviewed Dec 15, 2021

View reviewed changes

HansOlsson requested changes Dec 15, 2021

View reviewed changes

AHaumer requested review from HansOlsson and henrikt-ma December 15, 2021 17:45

HansOlsson reviewed Dec 16, 2021

View reviewed changes

Modelica/Resources/Reference/Modelica/Blocks/Examples/DemoMultiAndOr/comparisonSignals.txt Show resolved Hide resolved

HansOlsson approved these changes Dec 16, 2021

View reviewed changes

christiankral reviewed Dec 22, 2021

View reviewed changes

dietmarw removed their request for review January 18, 2022 15:16

tobolar removed their request for review January 27, 2022 08:30

MartinOtter approved these changes Mar 18, 2022

View reviewed changes

AHaumer and others added 5 commits January 18, 2024 07:35

Implemented and tested MultiAnd / MultiOr

df1642d

switched from connector with automatic sizing to vector input

22c63fa

Update Modelica/Blocks/Logical.mo

6d9d418

Co-authored-by: Henrik Tidefelt <[email protected]>

switched back to connectorSizing, implemented MultiNand and MultiNor

1c9f176

added MultiNand and MultiNor to reference results

823b8eb

beutlich force-pushed the MultiAndOr branch from e299c8a to 823b8eb Compare January 18, 2024 06:35

henrikt-ma reviewed Jan 22, 2024

View reviewed changes

Modelica/Blocks/Logical.mo Show resolved Hide resolved

henrikt-ma reviewed Jan 22, 2024

View reviewed changes

Modelica/Blocks/Logical.mo Show resolved Hide resolved

henrikt-ma reviewed Jan 22, 2024

View reviewed changes

henrikt-ma requested changes Jan 22, 2024

View reviewed changes

HansOlsson reviewed Jan 22, 2024

View reviewed changes

henrikt-ma reviewed Jan 22, 2024

View reviewed changes

HansOlsson reviewed Jan 22, 2024

View reviewed changes

	The output is <strong>true</strong> if at least one input is <strong>false</strong>, otherwise
	The output is <strong>true</strong> if any of the inputs is <strong>false</strong>, otherwise

	</p>
	<em>Note that the result is not the negation of <strong>MultiAnd</strong> in the case of an empty input vector.</em>
	</p>

	y = not ((u1 and u2) or (not u1 and not u2));
	y = (u1 and not u2) or (not u1 and u2);

	The output is <strong>true</strong> if at least one element of the input vector is <strong>false</strong>, otherwise
	The output is <strong>true</strong> if the input vector is empty or at least one element of the input vector is <strong>false</strong>, otherwise

	y=not Modelica.Math.BooleanVectors.allTrue(u);
	y=not Modelica.Math.BooleanVectors.andTrue(u);

Implemented and tested MultiAnd / MultiOr #3915

Are you sure you want to change the base?

Implemented and tested MultiAnd / MultiOr #3915

Conversation

AHaumer commented Dec 13, 2021

Choose a reason for hiding this comment

AHaumer Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HansOlsson Dec 15, 2021 • edited by tobolar Loading

Choose a reason for hiding this comment

henrikt-ma commented Dec 14, 2021

AHaumer commented Dec 14, 2021 • edited Loading

henrikt-ma commented Dec 14, 2021

henrikt-ma commented Dec 15, 2021

henrikt-ma left a comment

Choose a reason for hiding this comment

sjoelund commented Dec 15, 2021

HansOlsson left a comment

Choose a reason for hiding this comment

AHaumer commented Dec 15, 2021 • edited Loading

AHaumer commented Dec 15, 2021 • edited Loading

HansOlsson commented Dec 15, 2021

henrikt-ma commented Dec 15, 2021

HansOlsson commented Dec 15, 2021

AHaumer commented Dec 15, 2021

tobolar commented Dec 15, 2021

AHaumer commented Dec 15, 2021 • edited Loading

HansOlsson commented Dec 15, 2021

AHaumer commented Dec 15, 2021

henrikt-ma commented Dec 15, 2021

HansOlsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikt-ma left a comment

Choose a reason for hiding this comment

HansOlsson commented Jan 22, 2024

henrikt-ma commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AHaumer Dec 14, 2021 •

edited

Loading

HansOlsson Dec 15, 2021 •

edited by tobolar

Loading

AHaumer commented Dec 14, 2021 •

edited

Loading

AHaumer commented Dec 15, 2021 •

edited

Loading

AHaumer commented Dec 15, 2021 •

edited

Loading

AHaumer commented Dec 15, 2021 •

edited

Loading