Add double factorial function #72

chrisdamba · 2016-12-17T13:29:01Z

Resolves #44 .

Checklist

Please ensure the following tasks are completed before submitting this pull request.

Read, understood, and followed the contributing guidelines, including the relevant style guides.
Read and understand the Code of Conduct.
Read and understood the licensing terms.
Searched for existing issues and pull requests before submitting this pull request.
Filed an issue (or an issue already existed) prior to submitting this pull request.
Rebased onto latest develop.
Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

Provides an implementation for the double factorial function

Related Issues

Does this pull request have any related issues?

This pull request:

resolves Implement double factorial #44

Questions

Any questions for reviewers of this pull request?

Follow-up questions are in the responses to the review suggestions.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Encountering errors when trying to use factorial2 method in Scipy for generating the fixtures.

@stdlib-js/reviewers

…dlib into feature/factorial2

Planeshifter

Thank you very much for this pull request! I left some comments.

Planeshifter · 2016-12-17T17:26:48Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+The double factorial should not be confused with the factorial function iterated twice, written as (n!)! and not n!!
+
+The [double factorial][double-factorial-function] is implemented for integer values in terms of the [Factorial][@stdlib/math/base/special/factorial] function using the relation:


Missing [@stdlib/math/base/special/factorial]: https://github.com/stdlib-js/stdlib in the link section at the end of the file.

Planeshifter · 2016-12-17T17:29:54Z

lib/node_modules/@stdlib/math/base/special/factorial2/examples/index.js

+'use strict';
+
+var incrspace = require( '@stdlib/math/generics/utils/incrspace' );
+var factorial = require( './../lib' );


factorial should be replaced by factorial2 in this file.

Planeshifter · 2016-12-17T17:34:03Z

lib/node_modules/@stdlib/math/base/special/factorial2/lib/factorial2.js

+function factorial2( x ) {
+	var n;
+	if (
+		(isInteger( x ) && x < 0) ||


Use isNegativeInteger module instead.

Alternatively, keep isInteger, but cache the result so you can use the boolean later at line 66.

var isInt; isInt = isInteger( x ); if ( isInt && x < 0 ) { return NaN; } ... if ( isInt ) { ... }

Good call :)

Planeshifter · 2016-12-17T17:38:42Z

lib/node_modules/@stdlib/math/base/special/factorial2/package.json

@@ -0,0 +1,38 @@
+{


The contents of the package.json files have changed, take a look at the snippet. Fields that need to be updated: author, contributors, repository, homepage, bugs, and license.

Not so sure what values that need to be updated for this.

{ "name": "@stdlib/math/base/special/factorial2", "version": "0.0.0", "description": "Evaluate a double factorial.", "author": { "name": "The Stdlib Authors", "url": "https://github.com/stdlib-js/stdlib/graphs/contributors" }, "contributors": [ { "name": "The Stdlib Authors", "url": "https://github.com/stdlib-js/stdlib/graphs/contributors" } ], "scripts": {}, "main": "./lib", "repository": { "type": "git", "url": "git://github.com/stdlib-js/stdlib.git" }, "homepage": "https://github.com/stdlib-js/stdlib", "keywords": [ "stdlib", "stdmath", "mathematics", "math", "special functions", "special", "function", "factorial", "double factorial", "fact", "combinatorics", "gamma", "number" ], "bugs": { "url": "https://github.com/stdlib-js/stdlib/issues" }, "dependencies": {}, "devDependencies": {}, "engines": { "node": ">=0.10.0" }, "license": "Apache-2.0" }

Planeshifter · 2016-12-17T17:40:16Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/test.js

+
+var integers = require( './fixtures/julia/integers.json' );
+var decimals = require( './fixtures/julia/decimals.json' );
+


Add empty line before // TESTS //.

Planeshifter · 2016-12-17T17:45:40Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/test.js

+		} else {
+			delta = abs( v - expected[ i ] );
+			tol = EPS * Math.max( 1.0, abs( v ), abs( expected[ i ] ) );
+			t.ok( delta <= tol, 'within tolerance. x: ' + x[ i ] + '. Value: ' + v + '. Expected: ' + expected[ i ] + '. Tolerance: ' + tol + '.' );


Can you replace the last two lines with

tol = EPS * abs( expected[ i ] ); t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+v+'. E: '+expected[i]+'. Δ: '+delta+'. tol: '+tol );

and check whether the tests still pass?

This passes.

Planeshifter · 2016-12-17T17:46:13Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/test.js

+		v = factorial2( x[ i ] );
+		delta = abs( v - expected[ i ] );
+		tol = 2.0e-14 * Math.max( 1.0, abs( v ), abs( expected[ i ] ) );
+		t.ok( delta <= tol, 'within tolerance. x: ' + x[ i ] + '. Value: ' + v + '. Expected: ' + expected[ i ] + '. Tolerance: ' + tol + '.' );


Can you replace the last two lines with

tol = 2.0e-14 * abs( expected[ i ] ); t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+v+'. E: '+expected[i]+'. Δ: '+delta+'. tol: '+tol );

and check whether the tests still pass?

The tolerance should be scaled via epsilon.

tol = <scalar> * EPS * abs( expected[ i ] );

If I scale the tolerance with epsilon, the tests break.

Can you explain the rationale behind scaling the tolerance with epsilon? Also, why can't we apply the same for the factorial tests?

kgryte · 2016-12-17T17:54:57Z

Awesome. @chrisdamba Can you also complete the PR template checklist and update the description section.

kgryte · 2016-12-17T17:57:16Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+<!-- </equation> -->
+
+The double factorial should not be confused with the factorial function iterated twice, written as (n!)! and not n!!


Note that the double factorial should not...

kgryte · 2016-12-17T17:57:46Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+    <br>
+</div>
+
+The [Gamma][@stdlib/math/base/special/gamma] function extends the [double factorial][double-factorial-function] function for non-integer values.


...function to non-integer values.

kgryte · 2016-12-17T18:00:08Z

lib/node_modules/@stdlib/math/base/special/factorial2/lib/factorial2.js

+var DOUBLE_FACTORIALS = require( './doublefactorials.json' );
+var NINF = require( '@stdlib/math/constants/float64-ninf' );
+var PINF = require( '@stdlib/math/constants/float64-pinf' );
+


Add another newline.

kgryte · 2016-12-17T18:01:46Z

lib/node_modules/@stdlib/math/base/special/factorial2/lib/factorial2.js

+var isInteger = require( '@stdlib/math/base/utils/is-integer' );
+var isNegativeZero = require( '@stdlib/math/base/utils/is-negative-zero' );
+var sqrt = require( '@stdlib/math/base/special/sqrt' );
+var DOUBLE_FACTORIALS = require( './doublefactorials.json' );


This should be the last require statement. Order of requires:

Builtins; e.g., path, Buffer, etc.

External packages; e.g., debug, object-keys, etc.

Internal packages; e.g., @stdlib/math/base/special/sqrt.

Relative modules; e.g., ./doublefactorials.json.

kgryte · 2016-12-17T18:09:27Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/fixtures/julia/runner.jl

@@ -0,0 +1,97 @@
+#!/usr/bin/env julia


Might be better to use Python and SciPy for generating test fixtures, as SciPy actually has a factorial2 implementation against which we can test.

I think so too but I've discovered some errors whilst trying to generate the fixtures, as in the attached screenshot.

Will try to reproduce locally.

Did some investigating. Based on the SciPy and Boost implementations, the double factorial is simply not implemented for doubles. In the code above, if you cast x to integers, then the fixtures work.

As a result, unlikely to get either of those two reference implementations to provide test fixtures.

Further, I am willing to hazard a guess that the Gamma relation is not straightforward. Boost only applies a Gamma approximation for odd integers. SciPy applies two different approximations based on whether an input integer is odd or even. While the double factorial can be extended to reals, neither of those two implementations can serve as reference implementations for that particular use case.

So in this case do we leave the fixtures as they are?

Based on my assessment, looks like the package implementation needs refactoring. Notably, unless we know of some implementation which actually does extend the double factorial function to all reals, we should stick to Boost and Scipy and only support integer inputs. For large inputs, similar to Boost, if not covered by the table, we can then apply a Gamma approximation*.

Once that is updated, the fixtures can be updated, and we'll have a better idea of where we stand.

NOTE: I would be curious to know how the table lookup compares to integer multiplication in terms of performance as done in SciPy. Did you ever investigate this?

Definitely concur, needs some refactoring. I haven't actually done the investigation but I believe it's worth doing the comparison. Will do so and update you the results.

Awesome. Thanks! Really appreciate your tending to this. 👍

kgryte · 2016-12-17T19:20:32Z

@chrisdamba Once you make the requested changes, if you could squash the commits into a single commit, that would be awesome! This would make the commit history a bit cleaner for us, so that we can identify when a unit of functionality was initially added.

Thanks again for all your work!

…the test fixtures

codecov-io · 2016-12-18T15:53:29Z

Current coverage is 88.2077% (diff: 96.6666%)

Merging #72 into develop will increase coverage by 0.0086%

@@            develop        #72   diff @@
==========================================
  Files          2202       2204     +2   
  Lines         29201      29231    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          25755      25784    +29   
- Misses         3446       3447     +1   
  Partials          0          0

Diff Coverage	File Path
••••••••• 96%	new ...tdlib/math/base/special/factorial2/lib/factorial2.js
•••••••••• 100%	new ...es/@stdlib/math/base/special/factorial2/lib/index.js

Powered by Codecov. Last update 875afa1...a7fb88f

kgryte · 2016-12-18T18:39:01Z

lib/node_modules/@stdlib/math/base/special/factorial2/package.json

@@ -0,0 +1,38 @@
+{


{ "name": "@stdlib/math/base/special/factorial2", "version": "0.0.0", "description": "Evaluate a double factorial.", "author": { "name": "The Stdlib Authors", "url": "https://github.com/stdlib-js/stdlib/graphs/contributors" }, "contributors": [ { "name": "The Stdlib Authors", "url": "https://github.com/stdlib-js/stdlib/graphs/contributors" } ], "scripts": {}, "main": "./lib", "repository": { "type": "git", "url": "git://github.com/stdlib-js/stdlib.git" }, "homepage": "https://github.com/stdlib-js/stdlib", "keywords": [ "stdlib", "stdmath", "mathematics", "math", "special functions", "special", "function", "factorial", "double factorial", "fact", "combinatorics", "gamma", "number" ], "bugs": { "url": "https://github.com/stdlib-js/stdlib/issues" }, "dependencies": {}, "devDependencies": {}, "engines": { "node": ">=0.10.0" }, "license": "Apache-2.0" }

kgryte · 2016-12-18T18:41:14Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+Note that the double factorial should not be confused with the factorial function iterated twice, written as (n!)! and not n!!
+
+The [double factorial][double-factorial-function] is implemented for integer values in terms of the [Factorial][@stdlib/math/base/special/factorial] function using the relation:


Lowercase "factorial":

...in terms of the factorial function using the relation:

kgryte · 2016-12-18T18:41:33Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+The [double factorial][double-factorial-function] is implemented for integer values in terms of the [Factorial][@stdlib/math/base/special/factorial] function using the relation:
+
+<!-- <equation class="equation" label="eq:double_factorial_function_and_factorial" align="center" raw="(2n)!! = 2^n * n!" alt="Factorial function extension via the Factorial function"> -->


Lowercase "Factorial"

2^n \cdot n!

kgryte · 2016-12-18T18:42:24Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+The [Gamma][@stdlib/math/base/special/gamma] function extends the [double factorial][double-factorial-function] function to non-integer values.
+
+<!-- <equation class="equation" label="eq:double_factorial_function_and_gamma" align="center" raw="(2n-1)!! = \Gamma \left( \frac{(2n+1)}{2} \right) * \frac{2^n}{\sqrt{\pi}}" alt="Factorial function extension via the Gamma function"> -->


I do not believe you need the * (asterisk) in your equation. This applies here and immediately below.

kgryte · 2016-12-18T18:43:13Z

lib/node_modules/@stdlib/math/base/special/factorial2/README.md

+
+<!-- <equation class="equation" label="eq:double_factorial_function_and_factorial" align="center" raw="(2n)!! = 2^n * n!" alt="Factorial function extension via the Factorial function"> -->
+
+<div class="equation" align="center" data-raw-text="(2n)!! = 2^n * n!" data-equation="eq:double_factorial_function_and_factorial">


2^n \cdot n!

kgryte · 2016-12-18T18:45:06Z

lib/node_modules/@stdlib/math/base/special/factorial2/lib/factorial2.js

+* // returns NaN
+*/
+function factorial2( x ) {
+	var n;


Variables should be listed in order of length.

var isInt; var n;

kgryte · 2016-12-18T18:47:52Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/fixtures/julia/runner.jl

@@ -0,0 +1,97 @@
+#!/usr/bin/env julia


Will try to reproduce locally.

kgryte · 2016-12-18T18:51:41Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/test.js

@@ -96,7 +97,7 @@ tape( 'the function evaluates the double factorial function (positive integers <
 		} else {
 			delta = abs( v - expected[ i ] );
 			tol = EPS * Math.max( 1.0, abs( v ), abs( expected[ i ] ) );


This line should be

tol = EPS * abs( expected[ i ] );

kgryte · 2016-12-18T18:58:16Z

lib/node_modules/@stdlib/math/base/special/factorial2/test/test.js

@@ -126,7 +127,7 @@ tape( 'the function evaluates the factorial function (decimal values)', function
 		v = factorial2( x[ i ] );
 		delta = abs( v - expected[ i ] );
 		tol = 2.0e-14 * Math.max( 1.0, abs( v ), abs( expected[ i ] ) );


This line should be

tol = <num> * EPS * abs( expected[ i ] );

For example,

tol = 100.0 * EPS * abs( expected[ i ] );

The reason why we scale epsilon is that doing so provides more context as to how much results are off by and lessens the presence of so-called "magic" numbers. For instance, if we know that

actual = <number>; expected = 1.0; tol = 1.0 * EPS * abs( expected ); delta = abs( actual - expected ); bool = ( delta <= tol );

we know that actual value differs from the expected by only 1 bit. Similarly for other values, although the correspondence between units epsilon and number of differing bits is not 1:1.

Great explanation, thank you for this!

No problem! :)

kgryte · 2016-12-19T01:27:48Z

lib/node_modules/@stdlib/math/base/special/factorial2/lib/factorial2.js

+// VARIABLES //
+
+var MAX_FACTORIAL = 170; // TODO: consider extracting as a constant
+var PIO2 = 1.57079632679489661923; // pi/2


This can be replaced by using @stdlib/math/constants/float64-half-pi.

I can see there are other areas in the library which need to be updated as well to use the constant e.g in acos, asin and atan.

Indeed. We have an internal backlog of various clean-up tasks. :( Currently picking them off as we have time. Thanks for the reminder!

Wouldn't mind picking up this clean-up task if there's an issue not assigned.

We can create an issue to this end, and, if you want to submit a separate PR addressing them, that would be great!

@chrisdamba See issue #74.

rreusser · 2017-05-06T21:41:49Z

I'd love to help get this merged. Am I to conclude that, at least for a start, restricting the implementation to integers is the right move? Is the function then strictly a lookup table?

Planeshifter · 2017-05-06T21:48:37Z

Yes, restricting to integer inputs should be fine.

A table lookup and for large inputs a Gamma approximation should work, as Athan pointed out. Maybe have a look at how Boost does it.

kgryte · 2023-09-21T21:04:28Z

Closing this PR due to inactivity.

chrisdamba added 5 commits December 6, 2016 20:33

WIP - implementing double factorial stdlib-js#44

521348b

WIP - implementing double factorial stdlib-js#44

1981a82

Merge branch 'feature/factorial2' of https://github.com/chrisdamba/st…

0ca9942

…dlib into feature/factorial2

implementing double factorial

6214caa

fixed julia typo in function name

a7fb88f

Planeshifter requested changes Dec 17, 2016

View reviewed changes

kgryte added feature Feature requests. math Specific to mathematical functionality. labels Dec 17, 2016

kgryte reviewed Dec 17, 2016

View reviewed changes

Requested changes minus python runner and scaling epsilon for one of …

3b642e7

…the test fixtures

kgryte requested changes Dec 18, 2016

View reviewed changes

kgryte reviewed Dec 19, 2016

View reviewed changes

chrisdamba changed the title ~~Feature/factorial2~~ Add double factorial function Dec 20, 2016

Planeshifter closed this May 6, 2017

Planeshifter reopened this May 6, 2017

kgryte closed this Sep 21, 2023

kgryte mentioned this pull request Oct 24, 2023

Add double-factorial implementation #1112

Merged

1 task

Dec	JAN	Feb
	12
2023	2024	2025


		The double factorial should not be confused with the factorial function iterated twice, written as (n!)! and not n!!

		The [double factorial][double-factorial-function] is implemented for integer values in terms of the [Factorial][@stdlib/math/base/special/factorial] function using the relation:


		var integers = require( './fixtures/julia/integers.json' );
		var decimals = require( './fixtures/julia/decimals.json' );


		<!-- </equation> -->

		The double factorial should not be confused with the factorial function iterated twice, written as (n!)! and not n!!


		The [double factorial][double-factorial-function] is implemented for integer values in terms of the [Factorial][@stdlib/math/base/special/factorial] function using the relation:

		<!-- <equation class="equation" label="eq:double_factorial_function_and_factorial" align="center" raw="(2n)!! = 2^n * n!" alt="Factorial function extension via the Factorial function"> -->


		The [Gamma][@stdlib/math/base/special/gamma] function extends the [double factorial][double-factorial-function] function to non-integer values.

		<!-- <equation class="equation" label="eq:double_factorial_function_and_gamma" align="center" raw="(2n-1)!! = \Gamma \left( \frac{(2n+1)}{2} \right) * \frac{2^n}{\sqrt{\pi}}" alt="Factorial function extension via the Gamma function"> -->


		<!-- <equation class="equation" label="eq:double_factorial_function_and_factorial" align="center" raw="(2n)!! = 2^n * n!" alt="Factorial function extension via the Factorial function"> -->

		<div class="equation" align="center" data-raw-text="(2n)!! = 2^n * n!" data-equation="eq:double_factorial_function_and_factorial">

Add double factorial function #72

Add double factorial function #72

Conversation

chrisdamba commented Dec 17, 2016 • edited

Checklist

Description

Related Issues

Questions

Other

Planeshifter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdamba Dec 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte commented Dec 17, 2016

Choose a reason for hiding this comment

kgryte Dec 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte Dec 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte Dec 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte commented Dec 17, 2016

codecov-io commented Dec 18, 2016

Current coverage is 88.2077% (diff: 96.6666%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented May 6, 2017

Planeshifter commented May 6, 2017

kgryte commented Sep 21, 2023

chrisdamba commented Dec 17, 2016 •

edited

chrisdamba Dec 18, 2016 •

edited

kgryte Dec 17, 2016 •

edited

kgryte Dec 17, 2016 •

edited

kgryte Dec 18, 2016 •

edited

kgryte Dec 19, 2016 •

edited