Skip to content

Add std.complex.coshisinh and remove uses of creal math.#5698

Closed
ibuclaw wants to merge 1 commit into
dlang:masterfrom
ibuclaw:complexmath
Closed

Add std.complex.coshisinh and remove uses of creal math.#5698
ibuclaw wants to merge 1 commit into
dlang:masterfrom
ibuclaw:complexmath

Conversation

@ibuclaw

@ibuclaw ibuclaw commented Aug 19, 2017

Copy link
Copy Markdown
Member

Small addition for dlang/dmd#7081

Everything else looks like just deleting support code from phobos.

@ibuclaw ibuclaw requested a review from kyllingstad as a code owner August 19, 2017 14:01
@dlang-bot

Copy link
Copy Markdown

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Comment thread std/complex.d Outdated
*/
Complex!real coshisinh(real y) @safe pure nothrow @nogc
{
import std.math : cosh, sinh;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this the most efficient way of performing the calculation? I'm only asking because the one in std.math is implemented differently, using exp()/expm1().

@ibuclaw ibuclaw Aug 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

std.math.coshisinh just inlines the cosh+sinh implementations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, but the purpose of that seems to be to reduce two exp() calls to one. Does the compiler perform the same optimisation in your case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Two calls would be made. One for cosh, one for sinh.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

std.math.coshisinh makes only one exp call. In fact, that seems to be its raison d'être.

@ibuclaw

ibuclaw commented Aug 22, 2017

Copy link
Copy Markdown
Member Author

Added one fast path.

I also noticed there could be a few more functions added here. log, exp and tan are some of the notable that should be easy to add, but missing.

@PetarKirov

Copy link
Copy Markdown
Member

I also noticed there could be a few more functions added here. log, exp and tan are some of the notable that should be easy to add, but missing.

@ibuclaw do you intend to add them in this PR, or is it now good to go?

@ibuclaw

ibuclaw commented Aug 28, 2017

Copy link
Copy Markdown
Member Author

I can add other functions in latter PRs.

I should alzo add another unittest for y <= 0.5 for the sake of coverage.

@wilzbach wilzbach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As this is technically a new symbol, it requires approval from @andralex

Comment thread std/complex.d
}
}

@safe pure nothrow @nogc unittest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about making this a publicly documented example?

Comment thread std/complex.d

Note:
$(D coshisinh) is included here for convenience and for easy migration of code
that uses $(REF _coshisinh, std,math).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the motivation behind the move?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Are we deprecating complex real?

Comment thread std/complex.d
static import std.math;
if (std.math.fabs(y) <= 0.5)
return Complex!real(std.math.cosh(y), std.math.sinh(y));
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant control flow

@andralex andralex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't be blocked on me as I'm not an expert.

@andralex

Copy link
Copy Markdown
Member

cc math people @9il @klickverbot @WalterBright

@wilzbach

Copy link
Copy Markdown
Contributor

@andralex this PR was needed for the complex deprecation. I incorporated the diff with my addressed comments in the complex deprecation PR (#6014), so this can be safely closed.

In the retroperspective: it's really a shame that this was open for so long without any feedback :/

@wilzbach wilzbach closed this Feb 11, 2018
@ibuclaw ibuclaw deleted the complexmath branch February 11, 2018 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants