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

Simplify logic for Math.tanh #412

Merged
merged 1 commit into from
Mar 1, 2016
Merged

Simplify logic for Math.tanh #412

merged 1 commit into from
Mar 1, 2016

Conversation

benjoffe
Copy link
Contributor

@benjoffe benjoffe commented Mar 1, 2016

Previous PR for tanh left some code that was no longer needed (the checks against Infinity are redundant with the early exit).

if (b === Infinity) { return -1; }
return (a - b) / (Math.exp(x) + Math.exp(-x));

return (Math.expm1(x) - Math.expm1(-x)) / (Math.exp(x) + Math.exp(-x));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I not sure 4 math function calls really should be needed here, but I'll trust it has some accuracy benefit or something so not changing.

@benjoffe
Copy link
Contributor Author

benjoffe commented Mar 1, 2016

You're correct about neglibile performance difference for Math.sign changes, reverted.

@benjoffe benjoffe changed the title Simplify logic for Math.sign and Math.tanh Simplify logic for Math.tanh Mar 1, 2016
@ljharb
Copy link
Collaborator

ljharb commented Mar 1, 2016

Thanks, this looks great - if it's not too much to ask, could you rebase this on latest master?

@ljharb ljharb merged commit 072c2e4 into paulmillr:master Mar 1, 2016
@Yaffle
Copy link
Contributor

Yaffle commented Mar 1, 2016

Math.tanh = function (x) {
  return Math.sign(x) / (1 + 2 / Math.expm1(2 * Math.abs(x)));
};

What about this one?

@ljharb
Copy link
Collaborator

ljharb commented Mar 1, 2016

@Yaffle i'd be happy to review a new PR that changed the implementation, with assorted tests (altho i'd prefer the edge cases be explicit)

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.

3 participants