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

Sinh Accuracy #555

Closed
wants to merge 22 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@cukejianya

cukejianya commented Jan 21, 2016

First commit to an open source ever. Whoohoo. Anyways, for sinh(x) where -1 < x < 1, I used the 17th taylor series to calculate sinh.

( For sinh(.99999), I got 1.175185762896213 )

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 25, 2016

Thanks for your contribution @cukejianya , and sorry for the late reply.

A few remarks:

  • for the number implementation of sinh, we should not use BigNumbers under the hood.
  • it's probably easier readable to write the taylor series as a simple for loop in a single function rather than your solution of recursing over two functions taylorExpansion and SummationFunc, these functions spread the logic in a hard to read way.
  • you've updated two unit tests testing sinh with units. I think it will be a good idea to also test it for numbers in the range of -1 to 1, as this is what we want to fix right now. Add a few unit tests for say sinh(-0.7), sinh(-0.3), sinh(0.3), sinh(0.7). You can validate the correct output by comparing against for example Mathematica, Matlab, or other trusted/proven math applications.

It's probably the easiest solution to just take the implementation of the GNU Scientific Library as @jrus pointed out. This looks like a fast solution and is battle tested (of course we should unit test it ourselves too). What do you think?

@jrus

This comment has been minimized.

jrus commented Feb 1, 2016

Since Javascript is not a compiled language, I would recommend precomputing those divisions and putting the resulting constants directly in the code. e.g.

var a0 = 1.666666666666667e-01; // 1 / 6
var a1 = 8.333333333333333e-03; // 1 / 120
var a2 = 1.984126984126984e-04; // 1 / 5040
var a3 = 2.755731922398589e-06; // 1 / 362880
var a4 = 2.505210838544172e-08; // 1 / 39916800
var a5 = 1.605904383682161e-10; // 1 / 6227020800
var a6 = 7.647163731819816e-13; // 1 / 1307674368000
var a7 = 2.811457254345521e-15; // 1 / 355687428096000

I’m not sure whether JITs can compile away temporary variable creation inside a little function like this. If not, it might even be faster to just stick these constants directly into the return statement, like:

function _sinh (x) {
  if (x === 0) {
    return 0;
  } else if (x < 1 && x > -1) {
    // For |x| < 1, evaluate a degree 17 Taylor expansion of sinh using Horner’s rule.
    var y = x*x;
    return x*(1
      + y*(1.666666666666667e-01 // 1 / 6
      + y*(8.333333333333333e-03 // 1 / 120
      + y*(1.984126984126984e-04 // 1 / 5040
      + y*(2.755731922398589e-06 // 1 / 362880
      + y*(2.505210838544172e-08 // 1 / 39916800
      + y*(1.605904383682161e-10 // 1 / 6227020800
      + y*(7.647163731819816e-13 // 1 / 1307674368000
      + y*(2.811457254345521e-15 // 1 / 355687428096000
      )))))))));
  } else {
    return (Math.exp(x) - Math.exp(-x)) / 2;
  }
}
@jrus

This comment has been minimized.

jrus commented Feb 1, 2016

One more thing, I suspect there will still be an accuracy problem when evaluating sin, sinh, etc. with complex arguments.

You might try testing various values and checking them against a reliable implementation.

You might again look to the GNU Scientific Library implementation for ideas about how to accurately implement complex sin / sinh for values near the origin.

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 3, 2016

@cukejianya are you still interested in this PR or shall we close it? If not, @jrus, would you be interested to put your implementation in a PR (and add unit tests)?

@cukejianya

This comment has been minimized.

cukejianya commented Feb 4, 2016

@josdejong I dont mind.

@jrus

This comment has been minimized.

jrus commented Feb 6, 2016

That one looks fine now, though the alignment is slightly funky.

Do you folks know what the relative speed in JITed code is between if (x < 1 && x > -1) and if abs(x) < 1?

I’d also put a comment in, along the lines of what I suggested above:

    // For |x| < 1, evaluate a degree 17 Taylor expansion of sinh using Horner’s rule.

I’m too lazy to fork the repo and make pull requests. :-)

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 9, 2016

I’m too lazy to fork the repo and make pull requests. :-)

thanks for your input, we'll improve the function.

@cukejianya

This comment has been minimized.

cukejianya commented Feb 10, 2016

@jrus @josdejong Is there anything else I need to do for this? I know @infusion is currently integrating complexjs. Should we wait till he is done before working on complex sin / sinh?

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 11, 2016

@cukejianya well, @jus presented us a ready made solution which just needs to be implemented and unit tested. Since you let us know that you will not finish this, it simply waits until me or someone else has time to pick it up. This can safely be implemented in conjunction with other work.

@infusion

This comment has been minimized.

Collaborator

infusion commented Mar 4, 2016

Spent not too much time on reading the GSL implementation, but as far as I can tell the arithmetic looks similar to how I implemented complex sin and sinh. They don't have special code for abs(x) < 1, but maybe they rely on a more accurate real sinh implementation. It would be great if you could test either the dev branch of math.js or directly complex.js. If it doesn't have the accuracy you strive for, we should think of removing Math.sinh again and replace it with a more precise implementation like presented here, using a tailor approximation.

@cukejianya

This comment has been minimized.

cukejianya commented Mar 15, 2016

@josdejong @infusion I would love to continue working on this if possible.

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 15, 2016

@cukejianya that would be great, thanks.

@cukejianya

This comment has been minimized.

cukejianya commented Mar 16, 2016

I think Math.sinh is accurate enough and maybe even more accurate than the GSL implementation. For Math.sinh(0.99999999999), I got 1.1752011936283706 which aligns with the answer in wolframalpha: http://www.wolframalpha.com/input/?i=sinh(.99999999999).

@cukejianya

This comment has been minimized.

cukejianya commented Mar 16, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 16, 2016

try other values between -1 and 1, you will see :D

@cukejianya

This comment has been minimized.

cukejianya commented Mar 16, 2016

So I tried ±0.7, ±0.5, ±0.3 and the new built-in es6 function Math.sinh was spot on. It also looks like its supported in all major platforms except for IE11, Safari 7, and iOS 7. https://kangax.github.io/compat-table/es6/#test-Math_methods_Math.sinh

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 16, 2016

Yes, we can use the native Math.sinh and when it doesn't exist provide a polyfill as provided by Jacob here: #555 (comment)

@cukejianya

This comment has been minimized.

cukejianya commented Mar 16, 2016

@josdejong @infusion First, I want to apologize for all those failed tests. It seems for node versions 0.12, Math.sinh is not as accurate as I initially thought. I guess they fix their accuracy in later versions. The question now is 1) do we withhold from using Math.sinh for now and just use the GSL implementation, 2) should we continue using Math.sinh and find away to check Math.sinh accuracy and if it meets a certain standard use it, 3) or do we not test for the accuracy that I am checking for in sinh.test.js

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 19, 2016

I think we should use math.sinh though as it's the right "long term" solution. We've seen some numerical inaccuracies in node v0.12 before. To deal with this in unit tests you can do two things:

  • find some inputs which do not result in a round-off error in node v0.12 (maybe 0.5 gives this problem, but 0.6 not).
  • see whether the result is approximately equal rather than exactly equal, like compare up to 14 digits instead of the whole number.
@infusion

This comment has been minimized.

Collaborator

infusion commented Mar 19, 2016

I totally agree with Jos. I think most users (where user really means use and not just endure) already work on a machine, which supports Math.sinh, either on mobile or Firefox, Chrome, or even some useful IE of the younger generation. So it's not even a bet on the future, we already have it.

josdejong added a commit that referenced this pull request Apr 3, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 3, 2016

I've fixed this in the develop branch by using ES6 Math functions when available, and if not, falling back on a simple implementation using exp (nothing fancy). This means that in practice you may see some inaccuracy issues on old versions of node (v0.10, v0.12).

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 5, 2016

I think we could have closed this PR long time ago already, I will close it now.

@josdejong josdejong closed this Nov 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment