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

Update MultipleResonance.mo #4351

Merged
merged 2 commits into from Mar 13, 2024
Merged

Conversation

hubertus65
Copy link
Member

Fixed to make type more clear, and make it work in Modelon Impact. Also, removed an unused line of code. This is tested in latest version of Modelon Impact, and Dymola, and works in both.

Fixed to make type more clear, and make it work in Modelon Impact. Also, removed an unused line of code.
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

ok for me

@Harisankar-Allimangalath Harisankar-Allimangalath merged commit 1f8e25f into master Mar 13, 2024
6 checks passed
@Harisankar-Allimangalath Harisankar-Allimangalath deleted the hubertus65-patch-1 branch March 13, 2024 05:02
Harisankar-Allimangalath pushed a commit to Harisankar-Allimangalath/ModelicaStandardLibrary_1 that referenced this pull request Mar 13, 2024
Fixed to make type more clear, and make it work in Modelon Impact. Also, removed an unused line of code.
@beutlich beutlich removed the request for review from christiankral March 13, 2024 06:39
@beutlich beutlich added example Issue only addresses example(s) L: Electrical.QuasiStatic Issue addresses Modelica.Electrical.QuasiStatic labels Mar 13, 2024
@HansOlsson
Copy link
Contributor

This does not work in Dymola 2024x, and I cannot see how it would work according to the specification.
It may have worked in earlier versions (depending on flags) where we were a bit too aggressive in optimizing multiplication by zero.

Longer explanation for why:
Modelica.Units.SI.ComplexImpedance is just a Complex with units added - and should thus be seen as a constructor call according to:

https://specification.modelica.org/master/overloaded-operators.html#overloaded-constructors

There is only one constructor-function in Complex - fromReal where the imaginary part has default-value 0; so the only match we consider would be:
Complex.'constructor'.fromReal(re=100+Complex.j*0, im=0) which fails as the first argument is Complex and not real.
Having "copy-constructor" for types may be an interesting idea, but it hasn't been added yet.

How to correct:

However, Modelica.Units.SI.ComplexImpedance(100) or Modelica.Units.SI.ComplexImpedance(100, 0) would be valid.

@Harisankar-Allimangalath
Copy link
Contributor

@hubertus65 @AHaumer can you please look in to the above suggestion and decide on how to proceed with this .

Thankyou

@beutlich
Copy link
Member

How to correct:

Addressed by #4374.

beutlich pushed a commit to Harisankar-Allimangalath/ModelicaStandardLibrary_1 that referenced this pull request May 24, 2024
Fixed to make type more clear, and make it work in Modelon Impact. Also, removed an unused line of code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Issue only addresses example(s) L: Electrical.QuasiStatic Issue addresses Modelica.Electrical.QuasiStatic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants