Sincos #108
Sincos #108
Conversation
|
Awesome!!!! |
|
Was actually super easy! (You might be disappointed when you actually look at the implementation 😄) It's really just a straight up interleaving of the two since the majority of the computation is just duplicated. |
kgryte
added
Feature
|
It did seem like perhaps the reduction would be worth factoring out into another module. As far as I can tell, it's just duplicated identically for the sin/cos/sincos modules. |
|
Yes, indeed. One thing on my todo-list is to move |
|
@Planeshifter yeah, it looks like some of the cos and sin modules have been linted differently but that they're in fact identical in function (as best as I can tell; it might be a problem if not!) |
|
|
||
| <section class="links"> | ||
|
|
||
| [sine]: https://en.wikipedia.org/wiki/Sine |
There was a problem hiding this comment.
Can we point to the base special function here?
[@stdlib/math/base/special/sin]: https://github.com/stdlib-js/stdlib
Ditto for cosine. This allows us to direct people to our internal docs before sending them outbound.
There was a problem hiding this comment.
So we would have:
[@stdlib/math/base/special/sin]: https://github.com/stdlib-js/stdlib
[@stdlib/math/base/special/cos]: https://github.com/stdlib-js/stdlib
| b.end(); | ||
| }); | ||
|
|
||
| bench( pkg+'::separate-in-place-evaluation', function benchmark( b ) { |
There was a problem hiding this comment.
separate-evaluation,in-place. This is not yet documented, but we allow specifying multiple namespaces by delineating each namespace with a comma. This allows us to group here, e.g., the in-place benchmarks together and the separate-evaluation benchmarks, etc.
| b.end(); | ||
| }); | ||
|
|
||
| bench( pkg+'::built-in-in-place', function benchmark( b ) { |
| x = ( 20.0 * rand_double() ) - 10.0; | ||
| y = sin( x ); | ||
| z = cos( x ); | ||
| if ( y != y ) { |
There was a problem hiding this comment.
Check also that z is not NaN, otherwise the compiler may remove L87.
There was a problem hiding this comment.
Ah, that's the purpose of those checks. ✅
There was a problem hiding this comment.
Yes. When benchmarking, you need to do things to prevent the compiler from optimizing your code away. One means of doing so is to always provide changing input and validate output.
| } | ||
| } | ||
| elapsed = tic() - t; | ||
| if ( y != y ) { |
| ``` | ||
| """ | ||
| function benchmark() | ||
| t = BenchmarkTools.@benchmark sincos( (20.0*rand()) - 10.0 ) samples=1e6 |
There was a problem hiding this comment.
Did you try running this? I believe this will not work since sincos is defined outside the macro. Prefix sincos with a $.
There was a problem hiding this comment.
Both work. With $ is faster, but by an amount that doesn't seem statistically significant.
There was a problem hiding this comment.
Interesting. What version of Julia and Benchmarktools are you using?
There was a problem hiding this comment.
julia> VERSION
v"0.5.1"
julia> BenchmarkTools.VERSION
v"0.5.1"
There was a problem hiding this comment.
I am on 0.5.2 for both and previously encountered errors when not using $ for non-inlined functions. In any event, if it works, it works.
There was a problem hiding this comment.
Haha patch version lol.
| * v = sincos( NaN ); | ||
| * // returns [ NaN, NaN ] | ||
| * | ||
| * var out = new Float64Array( 2 ); |
There was a problem hiding this comment.
Let's make this a separate example.
@example
var sincos = require( '@stdlib/math/base/special/sincos' );
...
| // MAIN // | ||
|
|
||
| /** | ||
| * Return the last three digits of `N` with `y = x - N*pi/2` so that `|y| < pi/2`. |
| @@ -0,0 +1,379 @@ | |||
| /* eslint-disable no-plusplus */ | |||
There was a problem hiding this comment.
It seems to fail linting without this.
There was a problem hiding this comment.
/Users/rreusser/projects/node/stdlib/stdlib/lib/node_modules/@stdlib/math/base/special/sincos/lib/kernel_rem_pio2.js
93:3 warning Comments should not begin with a lowercase character capitalized-comments
111:1 warning Function 'compute' has too many statements (105). Maximum allowed is 100 max-statements
128:22 error Unary operator '++' used no-plusplus
128:27 error Unary operator '--' used no-plusplus
135:31 warning Comments should not begin with a lowercase character capitalized-comments
198:5 warning Comments should not begin with a lowercase character capitalized-comments
311:3 warning Comments should not begin with a lowercase character capitalized-comments
356:23 error Unary operator '++' used no-plusplus
356:28 error Unary operator '++' used no-plusplus
373:3 warning Comments should not begin with a lowercase character capitalized-comments
✖ 10 problems (4 errors, 6 warnings)
0 errors, 5 warnings potentially fixable with the `--fix` option.
| 'use strict'; | ||
|
|
||
| /* | ||
| * The following copyright, license, and long comment were part of the original implementation available as part of [FreeBSD]{@link https://svnweb.freebsd.org/base/release/9.3.0/lib/msun/src/k_sin.c?view=co}. |
There was a problem hiding this comment.
Is this function pulled from fdlibm? If not, should be able to remove this.
There was a problem hiding this comment.
It's a fairly trivial blend of the two versions (from sin and cos) that are. A few multiplications are shared between the two so that I simply put them in the same function call. I've modified the comment to reflect that more precisely:
/*
* The following copyright and license were part of the original implementation available as part of FreeBSD implementations of [k_sin.c]{@link https://svnweb.freebsd.org/base/release/9.3.0/lib/msun/src/k_sin.c?view=co} and [k_cos.c]{@link https://svnweb.freebsd.org/base/release/9.3.0/lib/msun/src/k_cos.c?view=co}..
*
* The implementation follows the original sine and cosine kernels, but has been modified for JavaScript and combined into a single function.
*/
|
@Planeshifter May be good as well to move |
Planeshifter
left a comment
Planeshifter
left a comment
There was a problem hiding this comment.
Thanks for working on this, Ricky!
|
|
||
| Parameters | ||
| ---------- | ||
| [out]: array |
There was a problem hiding this comment.
We don't use square brackets in the REPL help texts, but a (optional) after the type instead. This should be out: Array|TypedArray|Object (optional), since one can supply any collection for out.
| * | ||
| * var out = new Float64Array( 2 ); | ||
| * | ||
| * var v = sincos( out, 0.0 ); |
There was a problem hiding this comment.
Re-declaration of v. Should remove the var.
There was a problem hiding this comment.
I've separated this into two examples so that this should no longer be a re-declaration. ✅
| /** | ||
| * Simultaneously computes the sine and cosine of a number. | ||
| * | ||
| * @param {array} [out] - destination array |
There was a problem hiding this comment.
Object types should always be capitalized, e.g. @param {Array}, @param {Function}, and @param {Object}.
There was a problem hiding this comment.
P.S. Besides these standard JSDoc type annotations, you might want to have a look at our custom type definitions, which live here: https://github.com/stdlib-js/stdlib/tree/develop/tools/docs/jsdoc/typedefs
| * Computes the sine and cosine on \\( \approx [-\pi/4, \pi/4] \\) (except on \\(-0\\)), where \\( \pi/4 \approx 0.7854 \\). | ||
| * | ||
| * @private | ||
| * @param {array} out - destination array |
There was a problem hiding this comment.
Would I be correct to make this Array|TypedArray|Object?
There was a problem hiding this comment.
Correct and wrapped in () because the value could be one of one or more different types/values.
There was a problem hiding this comment.
Yes, thanks for catching this. In case of multiple accepted types in JSDoc, they should be enclosed in parentheses: (Array|TypedArray|Object)
There was a problem hiding this comment.
Wait, (Array|TypedArray|Object) or {Array|TypedArray|Object}?
There was a problem hiding this comment.
Both.
{(Array|TypedArray|Object)}
There was a problem hiding this comment.
Okay, I've attempted to fix this in the source files. They are now to the best of my knowledge correct.
| * @param {number} x - input value (assumed to be bounded by `~pi/4` in magnitude) | ||
| * @param {number} y - tail of `x` | ||
| * @param {number} iy - indicates whether `y` is `0` (if `iy = 0`, `y` assumed to be `0`) | ||
| * @returns {array} sine and cosine (in radians) |
|
@kgryte My current thinking was to just house |
|
In fdlibm, it is an independent file. http://www.netlib.org/fdlibm/ |
|
Also, I cannot remember |
| // Distill `q[]` into `IQ[]` in reverse order... | ||
| z = q[ jz ]; | ||
| j = jz; | ||
| for ( i = 0; j > 0; i++, j-- ) { |
There was a problem hiding this comment.
Disable the line rule on L129.
// eslint-disable-line no-plusplus
There was a problem hiding this comment.
The linter does not like complex for loop iteration.
| // Set up `F[0]` to `F[jx+jk]` where `F[jx+jk] = IPIO2[jv+jk]`: | ||
| j = jv - jx; | ||
| m = jx + jk; | ||
| for ( i = 0; i <= m; i++, j++ ) { |
There was a problem hiding this comment.
Disable the lint rule on this line.
// eslint-disable-line no-plusplus
Can then remove top-level disabling of lint rule.
| * | 3 | -C | S | -1/T | | ||
| * | ||
| * | ||
| * @param {(Array|TypedArray|Object)} out - destination array |
There was a problem hiding this comment.
Is this now @private since it's only indirectly exposed through the variadic wrapper in main.js?
| var bench = require( '@stdlib/bench' ); | ||
| var randu = require( '@stdlib/math/base/random/randu' ); | ||
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | ||
| var pkg = require( './../package.json' ).name; |
There was a problem hiding this comment.
This require statement should be moved to L10.
| [ sin( x ), cos( x ) ]; | ||
| end | ||
|
|
||
|
|
| Parameters | ||
| ---------- | ||
| out: Array|TypedArray|Object (optional) | ||
| Destination array |
|
|
||
| Returns | ||
| ------- | ||
| y: array |
There was a problem hiding this comment.
Rather, should be Array|TypedArray|Object
| 0x4D7327, 0x310606, 0x1556CA, 0x73A8C9, 0x60E27B, 0xC08C6B | ||
| ]; | ||
|
|
||
| // Double precision array, obtained by cutting `pi/2` into `24` bits chunks... |
| /** | ||
| * Simultaneously computes the sine and cosine of a number. | ||
| * | ||
| * #### Method |
| dir = dirname( file ); | ||
|
|
||
| # Negative medium sized values: | ||
| x = linspace( -256.0*pi, 0.0, 4000 ) |
| gen( x, out ); | ||
|
|
||
| # Positive medium sized values: | ||
| x = linspace( 0.0, 256.0*pi, 4000 ) |
There was a problem hiding this comment.
Unfortunately, the Julia linter (https://lintjl.readthedocs.io/en/stable/messages/) is not as configurable as ESLint and, in fact, not configurable at all. So we are left manually enforcing these kinds of things.
| gen( x, out ); | ||
|
|
||
| # Negative large values: | ||
| x = linspace( -2.0^20*(pi/2.0), -2.0^60*(pi/2.0), 4000 ) |
| gen( x, out ); | ||
|
|
||
| # Positive large values: | ||
| x = linspace( 2.0^20*(pi/2.0), 2.0^60*(pi/2.0), 4000 ) |
| gen( x, out ); | ||
|
|
||
| # Negative huge values: | ||
| x = linspace( -2.0^60*(pi/2.0), -2.0^1000*(pi/2.0), 4000 ) |
| gen( x, out ); | ||
|
|
||
| # Positive huge values: | ||
| x = linspace( 2.0^60*(pi/2.0), 2.0^1000*(pi/2.0), 4000 ) |
| tol = EPS * abs( sine[i] ); | ||
| t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+y[0]+'. Expected: '+sine[i]+'. tol: '+tol+'. delta: '+delta+'.' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove empty line to be consistent with other test blocks.
| tol = EPS * abs( sine[i] ); | ||
| t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+y[0]+'. Expected: '+sine[i]+'. tol: '+tol+'. delta: '+delta+'.' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove empty line to be consistent with other test blocks.
|
@kgryte I've fixed the most recent issues. Some of these issues are minor style issues in the modules from which I copied the bulk of this module. What's the best way to address those? With a separate PR? By pointing them out to you so that you can fix them? No preference on my side. Just let me know. |
|
If you want to submit a separate PR with all those knits, that is fine with me! |
|
Will wait on Circle CI to finish one more round of testing before squashing and merging. Thanks for all your work on this, @rreusser! |
3 participants
Resolves #100.
Checklist
develop.developbranch.Description
This pull request:
sinandcosto compute the two simultaneously. The only tricky spot was that the tolerances for early bailout in the sin/cos kernels were2^-26vs2^-27. I opted for the larger of the two (2^-26) so that the other might require a bit of extra computation, but since it needs to do the reduction and call the actual kernel anyway, it didn't seem like a big deal.Related Issues
No.
Questions
No.
Other
The benchmarks are interesting. On the strictly js side, there are quite a few comparisons to be done. In short, it's indeed somewhat faster to compute these together as opposed to separately.
@stdlib-js/reviewers