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 new power block for raising an input to a user parameterized exponent #4006

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

mestinso
Copy link
Collaborator

@mestinso mestinso commented Jun 29, 2022

This block adds a key missing mathematical function that is not available in Modelica.Blocks.Math. It was previously discussed here: #3967

Note that ideally this block should be called "Power" while the existing block called "Power" should be called "Exponentiation". Potentially in the future a conversion script could be used to swap the naming.

@mestinso mestinso added discussion Discussion issue that it not necessarily related to a concrete bug or feature and removed discussion Discussion issue that it not necessarily related to a concrete bug or feature labels Jun 29, 2022
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

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

Please add a test model in ModelicaTest.Blocks together with the comparisonSignals.txt file of the reference variables.

@beutlich beutlich added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks labels Jul 4, 2022
@beutlich beutlich requested a review from MartinOtter July 4, 2022 18:37
@mestinso
Copy link
Collaborator Author

mestinso commented Jul 8, 2022

Please add a test model in ModelicaTest.Blocks together with the comparisonSignals.txt file of the reference variables.

Done

@mestinso mestinso closed this Jul 8, 2022
@mestinso mestinso reopened this Jul 8, 2022
@mestinso mestinso requested a review from beutlich July 8, 2022 19:41
@AHaumer
Copy link
Contributor

AHaumer commented Jul 31, 2022

Still missing: a test example.
Although you are right, the old Power block should be named Exponentiation and the new block should be named Power,
the name change is problematic - mybe even with a conversion script.

@mestinso
Copy link
Collaborator Author

Still missing: a test example.

Although you are right, the old Power block should be named Exponentiation and the new block should be named Power,

the name change is problematic - mybe even with a conversion script.

@AHaumer I added a test example in ModelicaTest. However, I didn't add an example within MSL itself because none of the other math blocks seem to have examples either. Let me know what you suggest.

Regarding the conversion script, agreed with your concerns. Potentially if I use a temporary name (PowerNew or PowerFuture) it would make the conversion easier in the future (Power-->Exponentiation and PowerNew-->Power).

@HansOlsson any thoughts/suggestions on a future rename/conversion? Are direct name swaps possible with the current conversion script specification?

@HansOlsson
Copy link
Contributor

Regarding the conversion script, agreed with your concerns. Potentially if I use a temporary name (PowerNew or PowerFuture) it would make the conversion easier in the future (Power-->Exponentiation and PowerNew-->Power).

@HansOlsson any thoughts/suggestions on a future rename/conversion? Are direct name swaps possible with the current conversion script specification?

A name swap should be possible with conversion scripts.

However, there's also the issue of explaining it and user understanding, and I think a name swap will just create a mess for that, and I would prefer if we didn't plan to do that.

At least they are not plug-compatible so users will at least notice if they make an error.

@mestinso
Copy link
Collaborator Author

mestinso commented Sep 16, 2022

A name swap should be possible with conversion scripts.

However, there's also the issue of explaining it and user understanding, and I think a name swap will just create a mess for that, and I would prefer if we didn't plan to do that.

At least they are not plug-compatible so users will at least notice if they make an error.

Any other naming suggestions? Assuming there is no issue on a direct conversion script name swap, then I think probably the best names are maybe as is in the PR or change to Pow.

A few additional thoughts/notes here:

  • I would describe the current situation as confusing. I think the u^p operation is much more prevalent than a b^u operation (not counting the exp operation which is used all over the place). I know when I was first looking for a u^p operation block, I naturally grabbed the Power block and to my surprise it didn't behave as expected. Took me a while to realize one didn't exist.
  • I suspect the existing Power block isn't used too heavily, but maybe i'm biased by my own use cases. With that said, I'm not seeing any usage whatsoever in MSL or Buildings or a few different commercial libraries at least.
  • Given the above two comments, I don't think a future conversion would hurt too many people or cause too much confusion, if anything hopefully it would reduce confusion and drive better clarity long term.

@beutlich beutlich removed their request for review September 24, 2022 10:20
@beutlich beutlich removed their request for review October 14, 2022 19:30
@mestinso
Copy link
Collaborator Author

@MartinOtter @AHaumer Any additional concerns here? Or can we approve and merge?

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.

Seems ok.

@mestinso mestinso merged commit 410d60b into modelica:master Dec 1, 2022
@dietmarw dietmarw added this to the MSL4.1.0 milestone Dec 2, 2022
@beutlich beutlich changed the title Added new power block for raising an input to a user parameterized exponent Add new power block for raising an input to a user parameterized exponent 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.

6 participants