Skip to content
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

Add assert condition block #3985

Merged
merged 16 commits into from
Aug 26, 2022

Conversation

mestinso
Copy link
Collaborator

@mestinso mestinso commented May 11, 2022

  • Block is useful for applying asserts via graphical layer (rather than via equations)

@mestinso mestinso force-pushed the feature/assert-condition-block branch from c845f9c to 2525db4 Compare May 11, 2022 19:55
@dietmarw dietmarw added the L: Blocks Issue addresses Modelica.Blocks label May 12, 2022
@dietmarw dietmarw added this to the MSL4.1.0 milestone May 12, 2022
@HansOlsson
Copy link
Contributor

Investigating origin:

TerminateSimulation was added in 952520d (2007-03-25)

And BooleanExpression in ee9a1a2 (2004-07-07 - MSL 2.1Beta1 )

@mestinso mestinso force-pushed the feature/assert-condition-block branch 2 times, most recently from f4360cc to 9831065 Compare June 28, 2022 03:19
@mestinso
Copy link
Collaborator Author

The block has been restructured based on the previous discussion. Note that the initial scale of the block is 0.04 to be in alignment with the default height of the BooleanExpression block, which this block is expected to be paired with typically. See the picture below (top is the default rather than the bottom):

image

@mestinso mestinso force-pushed the feature/assert-condition-block branch from 9831065 to 509b2ec Compare June 28, 2022 03:59
@henrikt-ma
Copy link
Contributor

Note that the initial scale of the block is 0.04 to be in alignment with the default height of the BooleanExpression block, which this block is expected to be paired with typically. See the picture below (top is the default rather than the bottom):

Instead of using unusual initialScale, I think the best way to get more consistent font sizes is to stick to what other icons to (typically 0.1), and instead reduce the extent.

That said, I think I'd prefer the full 20x20 size, although the text assert should probably be made smaller to look nice.

@mestinso
Copy link
Collaborator Author

Note that the initial scale of the block is 0.04 to be in alignment with the default height of the BooleanExpression block, which this block is expected to be paired with typically. See the picture below (top is the default rather than the bottom):

Instead of using unusual initialScale, I think the best way to get more consistent font sizes is to stick to what other icons to (typically 0.1), and instead reduce the extent.

That said, I think I'd prefer the full 20x20 size, although the text assert should probably be made smaller to look nice.

A couple notes here:

  • Since I was extending from a partial model, it seemed easier to use initialScale approach. Various other blocks in this part of the library also used that approach (Modelica.Blocks.Interfaces.PartialRealMISO, Modelica.Blocks.Interfaces.PartialBooleanSISO_small)
  • For the text, I also followed the precedent for all other blocks in Modelica.Blocks.Logical, which had the same text extent: {{-90,40},{90,-40}}. I could drop it down to +- 85 or 80, but it seemed more aligned to just copy the precedent already set. Note that Modelica.Blocks.Logical.FallingEdge has a similarly long word (falling) with the same text extent.

I'm open to changes, but just wanted to make you aware of why I made the choices I did. Basically they aren't new choices, just following previous examples and choices already made.

@tobolar
Copy link
Contributor

tobolar commented Jun 28, 2022

Since I was extending from a partial model, it seemed easier to use initialScale approach.

An option for icon could be to disable the icon of partial model in extends clause by annotation (IconMap(primitivesVisible=false, extent={{-100,-100}, {100,100}})) and to define it similar to boolean expression (size, scaling, graphical elements).

@henrikt-ma
Copy link
Contributor

I'm open to changes, but just wanted to make you aware of why I made the choices I did. Basically they aren't new choices, just following previous examples and choices already made.

OK, thanks for explaining the design. Matching the style of FallingEdge makes sense, and then my personal preference would simply be to also use the same initialScale. By doing so, font sizes will be at least somewhat consistent with other icons such as FallingEdge.

  • For the text, I also followed the precedent for all other blocks in Modelica.Blocks.Logical, which had the same text extent: {{-90,40},{90,-40}}. I could drop it down to +- 85 or 80, but it seemed more aligned to just copy the precedent already set. Note that Modelica.Blocks.Logical.FallingEdge has a similarly long word (falling) with the same text extent.

The problem with the extent, as I see it, is that text gets scaled to fill the width rather than the height, meaning that one gets all sorts of effective font sizes depending on what goes into the box. Anyway, I don't think this is the PR where we're supposed to come up with new icon text design guidelines, so copying an existing style is probably the best way to proceed.

- Block is useful for applying asserts via graphical layer (rather than via equations)
@mestinso mestinso force-pushed the feature/assert-condition-block branch from 509b2ec to fd38d41 Compare June 29, 2022 14:39
@mestinso
Copy link
Collaborator Author

Ok, so based on the discussion I went ahead and removed the initial scale annotation. Now by default the scale will match the 2nd example in the picture above.

I think the PR should be good to go at this point unless any additional concerns.

mestinso and others added 2 commits June 29, 2022 16:01
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function and icon seems fine. Just leaving some suggestions about wording.

@tobolar
Copy link
Contributor

tobolar commented Aug 3, 2022

@beutlich Is it allowed to link to ModelicaReference.Operators.'assert()' in the documentation? This could be helpful since the component fully use it.

@tobolar
Copy link
Contributor

tobolar commented Aug 3, 2022

I have changed the default of assertionLevel paremeter since I believe the component will mostly be used to trigger a warning, not to terminate the simulation completely.

@henrikt-ma
Copy link
Contributor

I have changed the default of assertionLevel paremeter since I believe the component will mostly be used to trigger a warning, not to terminate the simulation completely.

And you're sure it wouldn't make more sense to use the same default as the Modelica language assert?

@tobolar
Copy link
Contributor

tobolar commented Aug 18, 2022

@henrikt-ma No, I'm not. :-( I'll revert this.

@tobolar
Copy link
Contributor

tobolar commented Aug 18, 2022

@beutlich How about #3985 (comment) ?

@beutlich
Copy link
Member

@beutlich Is it allowed to link to ModelicaReference.Operators.'assert()' in the documentation? This could be helpful since the component fully use it.

Why not? See also #2108.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general idea is good, but I would like to change the message and the test-case as indicated.

mestinso and others added 2 commits August 24, 2022 08:47
Co-authored-by: Hans Olsson <HansOlsson@users.noreply.github.com>
mestinso and others added 2 commits August 24, 2022 10:07
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
@mestinso
Copy link
Collaborator Author

Made the minor changes as requested (documentation paragraph changes, removal of getInstanceName(), and changing the wording from terminate to abort). Let me know if anything else, otherwise, looking to approve and merge this this here.

Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Modelica/Blocks/Logical.mo Show resolved Hide resolved
Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to me.

@mestinso mestinso merged commit 02657f5 into modelica:master Aug 26, 2022
@beutlich beutlich added the enhancement New feature or enhancement label Aug 28, 2022
@beutlich beutlich changed the title Added assert condition block Add assert condition block Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants