Skip to content

Use boolean array reductions - #3919

Merged
Harisankar-Allimangalath merged 6 commits into
modelica:masterfrom
henrikt-ma:use-boolean-array-reductions
Jan 18, 2024
Merged

Harisankar-Allimangalath merged 6 commits into
modelica:masterfrom
henrikt-ma:use-boolean-array-reductions

Conversation

@henrikt-ma

@henrikt-ma henrikt-ma commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Opening this based on recent comment by @AHaumer.

The point of the change is to take advantage of optimized tool implementations of Boolean array reductions.

This takes advantages of tools' dedicated implementations of conjunction and disjunction of multiple values expressed as Boolean array reduction with min and max, respectively.

With the result being given by simple expressions, it seemed natural to also set Inline = true.
@henrikt-ma henrikt-ma added enhancement New feature or enhancement L: Math Issue addresses Modelica.Math labels Dec 14, 2021
@AHaumer

AHaumer commented Dec 15, 2021

Copy link
Copy Markdown
Contributor

@henrikt-ma Hmmm ... tried your solution with Dymola 2021x:
Function input had incorrect type in the definition equation result = size(b, 1) > 0 and min(b)
Argument number 1 in min(b) is of type Boolean, but it must be a subtype of Real.
@HansOlsson Have min(Boolean) and max(Boolean) been implemented in a newer version of Dymola?

@AHaumer

AHaumer commented Dec 15, 2021

Copy link
Copy Markdown
Contributor

Just a less elegant solution but also a one-liner (maybe intuitive to read due to recursion):
output Boolean result = if size(b,1)>1 then b[1] and myAndTrue(b[2:end]) else b[1];
@HansOlsson Should this work in Dymola?

@henrikt-ma

Copy link
Copy Markdown
Contributor Author

@henrikt-ma Hmmm ... tried your solution with Dymola 2021x: Function input had incorrect type in the definition equation result = size(b, 1) > 0 and min(b) Argument number 1 in min(b) is of type Boolean, but it must be a subtype of Real. @HansOlsson Have min(Boolean) and max(Boolean) been implemented in a newer version of Dymola?

It's valid at least as of Modelica 3.4: https://specification.modelica.org/maint/3.4/Ch10.html#reduction-expressions

@henrikt-ma

Copy link
Copy Markdown
Contributor Author

Just a less elegant solution but also a one-liner (maybe intuitive to read due to recursion): output Boolean result = if size(b,1)>1 then b[1] and myAndTrue(b[2:end]) else b[1]; @HansOlsson Should this work in Dymola?

I don't think a recursive implementation has any clear advantage over the old algorithm with iteration. The point or the PR is to make use of the simplicity and efficiency of array reductions.

(By the way, if one should make a recursive implementation, I think that the base case should be the empty array.)

@sjoelund

Copy link
Copy Markdown
Member

Yes, I agree that min(b) would be much nicer. It's a simple implementation and in the specification since 2017. If it's a problem that Dymola doesn't yet support it, wait a little while before merging until it supports it. Or test if min(e for e in b) works better.

@henrikt-ma

Copy link
Copy Markdown
Contributor Author

Yes, I agree that min(b) would be much nicer. It's a simple implementation and in the specification since 2017. If it's a problem that Dymola doesn't yet support it, wait a little while before merging until it supports it. Or test if min(e for e in b) works better.

Of the two alternatives, I'd prefer waiting for them to get the support in place. I see a clear risk of the reduction with iterator not being implemented as efficiently as the variant that looks like a function call.

@HansOlsson

Copy link
Copy Markdown
Contributor

@henrikt-ma Hmmm ... tried your solution with Dymola 2021x: Function input had incorrect type in the definition equation result = size(b, 1) > 0 and min(b) Argument number 1 in min(b) is of type Boolean, but it must be a subtype of Real. @HansOlsson Have min(Boolean) and max(Boolean) been implemented in a newer version of Dymola?

It will be fully implemented in Dymola 2023. I don't understand why it was missed earlier.

@HansOlsson HansOlsson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code is good, but as noted it would be preferable for us to wait until Dymola 2023 is released (well, or at least about to be released).

Comment thread Modelica/Math/BooleanVectors.mo Outdated
Addressing review comment by @beutlich.
@henrikt-ma
henrikt-ma requested a review from beutlich January 10, 2022 06:59
Comment thread Modelica/Math/BooleanVectors.mo Outdated
Comment thread Modelica/Math/BooleanVectors.mo Outdated

@beutlich beutlich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK from my side. But I am not sure if min(Boolean array) is more readable than the explicit algorithm.

@henrikt-ma

Copy link
Copy Markdown
Contributor Author

OK from my side. But I am not sure if it actually is min(Boolean array) is more readable than the explicit algorithm.

The motivation is twofold: Tools may have efficient implementations for the min-reduction, and the functional style definition makes the function easily inlineable.

@henrikt-ma

Copy link
Copy Markdown
Contributor Author

@HansOlsson, I suppose Dymola 2023 has been released by now, so that we can proceed here?

@HansOlsson HansOlsson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

@AHaumer AHaumer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fine with me

@arunkumar-narasimhan

arunkumar-narasimhan commented Jan 18, 2024

Copy link
Copy Markdown
Collaborator

@HansOlsson @MartinOtter, can this approved PR be labeled with 4.1.0 milestone?

@beutlich

Copy link
Copy Markdown
Member

I'd say, if ModelicaTest.Math.BooleanFunctions still succeeds it can be merged (with milestone set accordingly).

@HansOlsson

Copy link
Copy Markdown
Contributor

@HansOlsson @MartinOtter, can this approved PR be labeled with 4.1.0 milestone?

Yes.

@arunkumar-narasimhan arunkumar-narasimhan added this to the MSL4.1.0 milestone Jan 18, 2024
@Harisankar-Allimangalath

Copy link
Copy Markdown
Contributor

I'd say, if ModelicaTest.Math.BooleanFunctions still succeeds it can be merged (with milestone set accordingly).

yes the test still succeeds , and the reference files are matching

@Harisankar-Allimangalath
Harisankar-Allimangalath merged commit ceae187 into modelica:master Jan 18, 2024
@henrikt-ma
henrikt-ma deleted the use-boolean-array-reductions branch January 22, 2024 10:03
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: Math Issue addresses Modelica.Math

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants