ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Added new power block for raising an input to a user parameterized exponent

Open mestinso opened this issue 3 years ago • 5 comments

This block adds a key missing mathematical function that is not available in Modelica.Blocks.Math. It was previously discussed here: https://github.com/modelica/ModelicaStandardLibrary/discussions/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 avatar Jun 29 '22 02:06 mestinso

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

Done

mestinso avatar Jul 08 '22 19:07 mestinso

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 avatar Jul 31 '22 09:07 AHaumer

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?

mestinso avatar Sep 15 '22 23:09 mestinso

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.

HansOlsson avatar Sep 16 '22 06:09 HansOlsson

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.

mestinso avatar Sep 16 '22 19:09 mestinso

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

mestinso avatar Nov 29 '22 17:11 mestinso