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

Added Power block and generalized Log block #2221

Closed
wants to merge 5 commits into from
Closed

Added Power block and generalized Log block #2221

wants to merge 5 commits into from

Conversation

christiankral
Copy link
Contributor

See #2191

@christiankral christiankral changed the title Added Power block an generalized log block Added Power block and generalized log block Mar 27, 2017
@christiankral christiankral changed the title Added Power block and generalized log block Added Power block and generalized Log block Mar 27, 2017
@beutlich
Copy link
Member

Thank you very much for this PR.

The comment of Power.base should better read Base of power.

@beutlich beutlich added this to the MSL_next-MINOR-version milestone Mar 27, 2017
@beutlich beutlich added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks labels Mar 27, 2017
@beutlich beutlich self-assigned this Mar 27, 2017
@beutlich
Copy link
Member

beutlich commented Mar 28, 2017

  • Should bocks Modelica.Blocks.Math.Log10 and Modelica.Blocks.Math.Exp be marked as obsolete then?
  • There is no need to put u in parentheses in the documentation formula.
  • Should parameter base be Evaluate=true?

@christiankral
Copy link
Contributor Author

  • Personally I think that Modelica.Blocks.Math.Exp and Modelica.Blocks.Math.Log10 shall be kept (not being obsolete), as these mathematical functions are also available in Modelica and every calculator
  • I removed the parentheses: now base ^ u, see 391b3e7
  • I would also prefer to have the parameters with Evaluate=true, see 391b3e7

@beutlich
Copy link
Member

One more issue (also raised by @HansOlsson) is if the natural base should be considered explicitly, such that pow(e, u) maps to exp(u) and log(u)/log(e) maps to lo log(u).

@christiankral
Copy link
Contributor Author

How should "base is equal to e" be implemented? Check for a small delta deviation? How big shall the delta deviation be?

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.

The Log description still says:
"Output the natural (base e) logarithm of the input (input > 0 required)"

This is no longer correct, I would prefer something like:

"Output the logarithm (default base e) of the input (input > 0 required)"

And similarly in the documentation.

@HansOlsson
Copy link
Contributor

How should "base is equal to e" be implemented? Check for a small delta deviation? How big shall the delta deviation be?

That is a tricky question, but assuming Modelica.Math.log(Modelica.Constants.e)==1 we can ignore that for the logarithm.

For the exponent the issue is that e.g. exp(20)-Modelica.Constants.e^20 is not zero (obviously this may vary - but there are good reasons for this).

The relative difference is 1e-15, i.e. a factor 10 above round-off - and exponentiation gives the right answer, to obtain the right answer in that case we could use Modelica.Math.exp(u*Modelica.Math.log(base)). But if the base is exactly given (e.g. as 2) the other alternative is more exact.

The underlying reason is that log, exp, pow in modern C-implementations use infinite precision and then round.

@christiankral
Copy link
Contributor Author

@HansOlsson As Modelica.Math.log(base)==1 seems to give a pretty clear answer on the base being equal to e, why can't we just use the following code:

block Log "Output the logarithm (default base e) of the input (input > 0 required)"
  extends Interfaces.SISO;
  parameter Real base = Modelica.Constants.e "Base of logarithm" annotation(Evaluate=true);
equation 
  y = if Modelica.Math.log(base)==1 then Modelica.Math.log(u) else Modelica.Math.log(u)/Modelica.Math.log(base);
end Log;

block Power "Output the power to a base of the input"
  extends Interfaces.SISO;
  parameter Real base = Modelica.Constants.e "Base of power" annotation(Evaluate=true);
equation 
  y = if Modelica.Math.log(base)==1 then Modelica.Math.exp(u) else base ^ u;
end Power;

If you do not agree, could you please provide an alternative code snippet.

@christiankral
Copy link
Contributor Author

christiankral commented Mar 29, 2017

And similarly in the documentation.

The documentation already reads:

This blocks computes the output y as the logarithm to the parameter base of the input u: 
    y = log( u ) / log( base );
An error occurs if the elements of the input u are zero or negative.

@tbeu
Copy link
Contributor

tbeu commented Mar 29, 2017

I was thinking of an extra enumeration type for the base, either natural or user input.

@HansOlsson
Copy link
Contributor

For log just write (since division by 1 is exact):

block Log "Output the logarithm (default base e) of the input (input > 0 required)"
  extends Interfaces.SISO;
  parameter Real base = Modelica.Constants.e "Base of logarithm" annotation(Evaluate=true);
equation 
  y = Modelica.Math.log(u)/Modelica.Math.log(base);
end Log;

For Power it is cleaner if we just write

block Power "Output the power to a base of the input"
  extends Interfaces.SISO;
  parameter Real base = Modelica.Constants.e "Base of power" annotation(Evaluate=true);
equation 
  y = if base==Modelica.Constants.e then Modelica.Math.exp(u) else base ^ u;
end Power;

Except that also this variant has equality-comparison between reals which isn't legal, so we have to use y = Modelica.Math.exp(u*Modelica.Math.log(base)); - if we want to preserve backward compatibility, or base^u if we prefer clarity.

We could let the user decided (along the lines suggested by @tbeu ):

block Power "Output the power to a base of the input"
  extends Interfaces.SISO;
  parameter Real base = Modelica.Constants.e "Base of power" annotation(Evaluate=true);
  parameter Boolean useExp = true "Use exp function in implementation"  annotation(Evaluate=true);
equation 
  y = if useExp then Modelica.Math.exp(u*Modelica.Math.log(base)) else base ^ u;
end Power;

I am unsure if that difference is significant enough.

@christiankral
Copy link
Contributor Author

I think the Log block is quite good as @HansOlsson proposed it.

Considering the options of the Exp block I think that two options make sense:

  1. Just use the implementation y = base ^ u as the user can always decided to use the Exp block alternatively, if he wants to use the exponential function
  2. Use the implementation of @HansOlsson so that the user can make the decision within exchanging blocks; this is the version I committed on the development branch now

@beutlich
Copy link
Member

beutlich commented Apr 4, 2017

Looks good to me. Thank you for your contribution! Going to merge it soon.

@beutlich
Copy link
Member

beutlich commented Apr 4, 2017

Squashed and applied.

@beutlich beutlich closed this Apr 4, 2017
@otronarp
Copy link
Member

I realize that it's a bit late to have comments on a closed pull request but this block occupies the generic name Power, what about the y = u ^ n case? We recently had user request for such a thing.

@beutlich
Copy link
Member

It is not too late as long as it was not yet released.

@beutlich beutlich removed the request for review from sjoelund June 9, 2022 19:46
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.

None yet

5 participants