Conversation
| end And; | ||
|
|
||
| block MultiAnd "Logical and of Boolean vector elements" | ||
| extends Modelica.Blocks.Interfaces.PartialBooleanMISO; |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
@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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No it doesn't (I tried it).
There was a problem hiding this comment.
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>
<p>
The output is <strong>true</strong> if any of the elements of the input vector are <strong>true</strong>, otherwise
the output is <strong>false</strong>.
</p>
</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.)
|
Related to this, but not to be handled in this PR, isn't it a mistake that |
|
@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. |
OK, I'll do it for the sake of demonstrating the power of the language.
Good point, I turned that into a suggestion. |
|
When introducing |
henrikt-ma
left a comment
There was a problem hiding this comment.
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.
The normal would be to have
I don't see any vectorized calls in those. Also, reductions are not vectorized calls. |
HansOlsson
left a comment
There was a problem hiding this comment.
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.
|
@HansOlsson don't forget the case when you want to connect more than one vector. |
|
@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? |
I think we shouldn't add |
For me, defining it in terms of
Yes, let's forget |
|
Regarding multi-xor: https://electronics.stackexchange.com/questions/93713/how-is-an-xor-with-more-than-2-inputs-supposed-to-work |
|
@HansOlsson you're right, MultiNot is not necessary. |
@AHaumer Would in that case |
|
@tobolar I'll try that - I think that'll work, you're right! |
|
@HansOlsson sigh ... ok let's do it that way you suggest. |
Right, all the evidence suggests that at least the name |
HansOlsson
left a comment
There was a problem hiding this comment.
seems good, except for the comment about missing reference signals that aren't that critical.
| <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> |
There was a problem hiding this comment.
I am not sure of compare a vs b is the correct terminology. This is how I understand it:
- Compare
avsbmeans to me thatais on the vertical axis,bis on the horizontal axis - Compare
awithbmeans to me thataandbare plotted as a function of time (in the same graph) - Compare
aandbmeans to me the same as 2.
I am kindly asking for a native speaker to comment on this
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
| textString="nand")}), Documentation(info="<html> | ||
| <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 |
There was a problem hiding this comment.
This could be formulated more consistently with MultiOr (since there is always a non-zero number of inputs):
| 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 |
There was a problem hiding this comment.
I don't see that there has to be at non-zero number of inputs.
There was a problem hiding this comment.
We have u1 and u2, which adds up to two inputs, no more, no less.
| <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 was a problem hiding this comment.
There should be a warning about not being the negation of MultiNand:
| </p> | |
| <em>Note that the result is not the negation of <strong>MultiAnd</strong> in the case of an empty input vector.</em> | |
| </p> |
| <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 was a problem hiding this comment.
There should be a warning about not being the negation of MultiAnd:
| </p> | |
| <em>Note that the result is not the negation of <strong>MultiNand</strong> in the case of an empty input vector.</em> | |
| </p> |
| extends Blocks.Interfaces.partialBooleanSI2SO; | ||
| equation | ||
| y = not (u1 and u2); | ||
| y = not ((u1 and u2) or (not u1 and not u2)); |
There was a problem hiding this comment.
Wouldn't it be easier expressed like this?
| y = not ((u1 and u2) or (not u1 and not u2)); | |
| y = (u1 and not u2) or (not u1 and u2); |
henrikt-ma
left a comment
There was a problem hiding this comment.
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, |
| 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 |
There was a problem hiding this comment.
| 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 | |
| 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 |
Here is the current code of the PR. Where does it use allTrue?
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
So, shouldn't we fix that so that Nand is the same as Not And?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
| 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 |
There was a problem hiding this comment.
This is where we need to deal with the use of allTrue:
| 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 |
| block MultiNand "Logical nand of Boolean vector elements" | ||
| extends Modelica.Blocks.Interfaces.PartialBooleanMISO; | ||
| equation | ||
| y=not Modelica.Math.BooleanVectors.allTrue(u); |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
@AHaumer, could you please consider including this change?
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.