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

AdditionalSqrt #4452

Merged
merged 3 commits into from
Oct 24, 2024
Merged

AdditionalSqrt #4452

merged 3 commits into from
Oct 24, 2024

Conversation

Esther-Devakirubai
Copy link
Contributor

Backport #4396

@Esther-Devakirubai Esther-Devakirubai added L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath L: Media Issue addresses Modelica.Media labels Aug 16, 2024
@beutlich beutlich added this to the MSL4.1.0 milestone Aug 16, 2024
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

@Esther-Devakirubai #4396 affects four files, while this backport only affects two. I guess there are some additional commits missing

@Esther-Devakirubai
Copy link
Contributor Author

@Esther-Devakirubai #4396 affects four files, while this backport only affects two. I guess there are some additional commits missing

Will check.

@Esther-Devakirubai
Copy link
Contributor Author

@Esther-Devakirubai #4396 affects four files, while this backport only affects two. I guess there are some additional commits missing

Will check.

@casella I see only 2 files being affected by this.

1be54b5

Please correct me if I am wrong.

@maltelenz
Copy link
Contributor

@Esther-Devakirubai there are more commits in #4396 : https://github.com/modelica/ModelicaStandardLibrary/pull/4396/commits

but this PR only contains the contents of one of them.

@casella
Copy link
Contributor

casella commented Sep 17, 2024

@Esther-Devakirubai there are more commits in #4396 : https://github.com/modelica/ModelicaStandardLibrary/pull/4396/commits

but this PR only contains the contents of one of them.

Yes, the #4396 PR contains multple commits, but for some reason the cherry-picking only considered the first one

Remaining are: documentation, or combined with other fractional exponents.
Copy link
Contributor

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

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

Does still not have the same changes as #4396 which it claims to backport.

@maltelenz maltelenz self-requested a review October 15, 2024 09:50
Copy link
Contributor

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

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

Changes now match what it claims to backport.

I have no opinion on whether this should be backported, but @casella already said it was OK to backport, so approving.

Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

I don't get why .sqrt and not just sqrt. sqrt(v) is defined as the square root if v is Real or Integer, which is the case here. @HansOlsson, why the leading dot?

@HansOlsson
Copy link
Contributor

I don't get why .sqrt and not just sqrt. sqrt(v) is defined as the square root if v is Real or Integer, which is the case here. @HansOlsson, why the leading dot?

There is a comment explaining that on the line: "Real, not complex sqrt".

Due to lookup sqrt in Modelica.ComplexMath will find the complex Modelica.ComplexMath.sqrt-function, which isn't desired here. The complex exp and log functions work around it by using Modelica.Math.exp and Modelica.Math.log

@Esther-Devakirubai
Copy link
Contributor Author

@casella Can this be approved and Merged?

@HansOlsson HansOlsson requested a review from casella October 24, 2024 11:19
@casella
Copy link
Contributor

casella commented Oct 24, 2024

There is a comment explaining that on the line: "Real, not complex sqrt".

Due to lookup sqrt in Modelica.ComplexMath will find the complex Modelica.ComplexMath.sqrt-function, which isn't desired here. The complex exp and log functions work around it by using Modelica.Math.exp and Modelica.Math.log

I previously assumed that built-in functions had precedence in the look-up process, so they could not be shadowed. I obviously missed Section 5.6.1.1 saying: "The builtin classes are put into the unnamed root of the class tree".

@casella
Copy link
Contributor

casella commented Oct 24, 2024

@casella Can this be approved and Merged?

Yes, please go ahead. Thanks!

@maltelenz
Copy link
Contributor

maltelenz commented Oct 24, 2024

@casella Can this be approved and Merged?

Yes, please go ahead. Thanks!

@casella You requested changes earlier, so you have to actually approve :)

Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM

@casella casella merged commit f6037d4 into maint/4.1.x Oct 24, 2024
6 checks passed
@casella casella deleted the backport_PR4396 branch October 24, 2024 15:32
@casella
Copy link
Contributor

casella commented Oct 24, 2024

@casella You requested changes earlier, so you have to actually approve :)

😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath L: Media Issue addresses Modelica.Media
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants