Skip to content

feat: add C implementation for math/base/special/rising-factorial #4455

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

Merged

Conversation

DhruvArvindSingh
Copy link
Contributor

Progresses #649 .

Description

What is the purpose of this pull request?

This pull request:

double stdlib_base_rising_factorial( const double x, const int32_t n )

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

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

No.

Checklist

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


@stdlib-js/reviewers

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: passed
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: passed
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: passed
  - task: lint_javascript_tests
    status: passed
  - task: lint_javascript_benchmarks
    status: passed
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: passed
  - task: lint_c_examples
    status: passed
  - task: lint_c_benchmarks
    status: passed
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---

---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
  - task: run_javascript_examples
    status: na
  - task: run_c_examples
    status: na
  - task: run_cpp_examples
    status: na
  - task: run_javascript_readme_examples
    status: na
  - task: run_c_benchmarks
    status: na
  - task: run_cpp_benchmarks
    status: na
  - task: run_fortran_benchmarks
    status: na
  - task: run_javascript_benchmarks
    status: na
  - task: run_julia_benchmarks
    status: na
  - task: run_python_benchmarks
    status: na
  - task: run_r_benchmarks
    status: na
  - task: run_javascript_tests
    status: na
---
@stdlib-bot stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Jan 2, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Jan 2, 2025

Coverage Report

Package Statements Branches Functions Lines
math/base/special/rising-factorial $\color{green}229/229$
$\color{green}+100.00\%$
$\color{green}25/25$
$\color{green}+100.00\%$
$\color{green}2/2$
$\color{green}+100.00\%$
$\color{green}229/229$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---

---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
  - task: run_javascript_examples
    status: na
  - task: run_c_examples
    status: na
  - task: run_cpp_examples
    status: na
  - task: run_javascript_readme_examples
    status: passed
  - task: run_c_benchmarks
    status: na
  - task: run_cpp_benchmarks
    status: na
  - task: run_fortran_benchmarks
    status: na
  - task: run_javascript_benchmarks
    status: na
  - task: run_julia_benchmarks
    status: na
  - task: run_python_benchmarks
    status: na
  - task: run_r_benchmarks
    status: na
  - task: run_javascript_tests
    status: na
---
@DhruvArvindSingh DhruvArvindSingh changed the title feat: add C implementation for math/base/special/rising-factorial feat: add C implementation for math/base/special/rising-factorial Jan 2, 2025
@anandkaranubc anandkaranubc self-requested a review May 11, 2025 07:50
@anandkaranubc
Copy link
Contributor

/stdlib merge

@stdlib-bot stdlib-bot added bot: In Progress Pull request is currently awaiting automation. and removed bot: In Progress Pull request is currently awaiting automation. labels May 11, 2025
*
* ## Notice
*
* The original C++ code and copyright notice are from the [Boost library]{@link http://www.boost.org/doc/libs/1_64_0/boost/math/special_functions/factorials.hpp}. The implementation has been modified according to project conventions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The original C++ code and copyright notice are from the [Boost library]{@link http://www.boost.org/doc/libs/1_64_0/boost/math/special_functions/factorials.hpp}. The implementation has been modified according to project conventions.
* The original C++ code and copyright notice are from the [Boost library]{@link http://www.boost.org/doc/libs/1_64_0/boost/math/special_functions/factorials.hpp}. The implementation has been modified according to stdlib conventions.

double xc;
bool inv;

if ( stdlib_base_is_nan( x ) || !stdlib_base_is_integer( n ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( stdlib_base_is_nan( x ) || !stdlib_base_is_integer( n ) ) {
if ( stdlib_base_is_nan( x ) ) {

n is already and integer.

#include "stdlib/math/base/assert/is_integer.h"
#include "stdlib/math/base/assert/is_nan.h"
#include "stdlib/math/base/special/gamma_delta_ratio.h"
#include "stdlib/math/base/special/falling_factorial.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include "stdlib/math/base/special/falling_factorial.h"
#include <stdint.h>

#include "stdlib/math/base/assert/is_integer.h"
#include "stdlib/math/base/assert/is_nan.h"
#include "stdlib/math/base/special/gamma_delta_ratio.h"
#include "stdlib/math/base/special/falling_factorial.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include "stdlib/math/base/special/falling_factorial.h"
#include <stdbool.h>

nc = n;
xc = x;
inv = false;
if ( xc < 0.0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( xc < 0.0 ) {
// For `x < 0`, we really have a falling factorial, modulo a possible change of sign. Note that the falling factorial isn't defined for negative `n`, so we'll get rid of that case first:

nc = -nc;
inv = true;
}
result = ( ( nc & 1 ) ? -1.0 : 1.0 ) * stdlib_base_falling_factorial( -xc, nc );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result = ( ( nc & 1 ) ? -1.0 : 1.0 ) * stdlib_base_falling_factorial( -xc, nc );
result = ( (nc&1) ? -1.0 : 1.0 ) * stdlib_base_falling_factorial( -xc, nc );

Following the same negative spacing in main.js

xc = x;
inv = false;
if ( xc < 0.0 ) {
if ( nc < 0.0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( nc < 0.0 ) {
if ( nc < 0 ) {

nc is int32_t

}
return 0.0;
}
if ( ( xc < 1.0 ) && ( xc + nc < 0.0 ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( ( xc < 1.0 ) && ( xc + nc < 0.0 ) ) {
if ( ( xc < 1.0 ) && ( xc+nc < 0.0 ) ) {

}
if ( ( xc < 1.0 ) && ( xc + nc < 0.0 ) ) {
result = stdlib_base_gamma_delta_ratio( 1.0 - xc, -nc );
return ( nc & 1 ) ? -result : result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ( nc & 1 ) ? -result : result;
return ( nc&1 ) ? -result : result;

#include "stdlib/math/base/special/rising_factorial.h"
#include "stdlib/math/base/napi/binary.h"

// cppcheck-suppress shadowFunction
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// cppcheck-suppress shadowFunction

Comment on lines 17 to 30
*
*
* ## Notice
*
* The original C++ code and copyright notice are from the [Boost library]{@link http://www.boost.org/doc/libs/1_64_0/boost/math/special_functions/factorials.hpp}. The implementation has been modified for JavaScript.
*
* ```text
* (C) Copyright John Maddock 2006, 2010.
*
* Use, modification and distribution are subject to the
* Boost Software License, Version 1.0. (See accompanying file
* LICENSE or copy at http://www.boost.org/LICENSE_1_0.txt)
* ```
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
*
* ## Notice
*
* The original C++ code and copyright notice are from the [Boost library]{@link http://www.boost.org/doc/libs/1_64_0/boost/math/special_functions/factorials.hpp}. The implementation has been modified for JavaScript.
*
* ```text
* (C) Copyright John Maddock 2006, 2010.
*
* Use, modification and distribution are subject to the
* Boost Software License, Version 1.0. (See accompanying file
* LICENSE or copy at http://www.boost.org/LICENSE_1_0.txt)
* ```
*/

This is not needed in native.js as there isn't anything derived from the Boost library.

Comment on lines 43 to 57
*
* ## Notes
*
* - The rising factorial is defined as
*
* ```tex
* \operatorname{risingFactorial}(x, n) = x (x-1) (x-2) (x-3) \ldots (x-n+1)
* ```
*
* or equivalently
*
* ```tex
* \operatorname{risingFactorial}(x, n) = \frac{ \Gamma(x + n) }{ \Gamma(x) };
* ```
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
* ## Notes
*
* - The rising factorial is defined as
*
* ```tex
* \operatorname{risingFactorial}(x, n) = x (x-1) (x-2) (x-3) \ldots (x-n+1)
* ```
*
* or equivalently
*
* ```tex
* \operatorname{risingFactorial}(x, n) = \frac{ \Gamma(x + n) }{ \Gamma(x) };
* ```
*

* ```tex
* \operatorname{risingFactorial}(x, n) = \frac{ \Gamma(x + n) }{ \Gamma(x) };
* ```
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
* @private

"libraries": [],
"libpath": [],
"dependencies": [
"@stdlib/math/base/assert/is-integer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"@stdlib/math/base/assert/is-integer",


t = tic();
for ( i = 0; i < ITERATIONS; i++ ) {
y = stdlib_base_rising_factorial( x[ i % 100 ], n[ i % 100 ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
y = stdlib_base_rising_factorial( x[ i % 100 ], n[ i % 100 ] );
y = stdlib_base_rising_factorial( x[ i%100 ], n[ i%100 ] );


for ( i = 0; i < 100; i++ ) {
x[ i ] = ( 40.0 * rand_double() ) + 1.0;
n[ i ] = (int32_t)( 41.0 * rand_double() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
n[ i ] = (int32_t)( 41.0 * rand_double() );
x[ i ] = ( 100.0*rand_double() ) - 50.0;

Using similar values used in cpp benchmarks, for consistency.


for ( i = 0; i < 100; i++ ) {
x[ i ] = ( 40.0 * rand_double() ) + 1.0;
n[ i ] = (int32_t)( 41.0 * rand_double() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
n[ i ] = (int32_t)( 41.0 * rand_double() );
n[ i ] = (int32_t)( 50.0*rand_double() );

Same comment.


// MAIN //

bench( pkg, opts, function benchmark( b ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bench( pkg, opts, function benchmark( b ) {
bench( pkg+'::native', opts, function benchmark( b ) {

var i;

x = randu( 100, 1.0, 41.0 );
n = discreteUniform( 100, 0, 40 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
n = discreteUniform( 100, 0, 40 );
x = uniform( 100, -50.0, 50.0 );

var i;

x = randu( 100, 1.0, 41.0 );
n = discreteUniform( 100, 0, 40 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
n = discreteUniform( 100, 0, 40 );
n = discreteUniform( 100, -50, 50 );


b.tic();
for ( i = 0; i < b.iterations; i++ ) {
y = risingFactorial( x[ i % x.length ], n[ i % n.length ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
y = risingFactorial( x[ i % x.length ], n[ i % n.length ] );
y = risingFactorial( x[ i%x.length ], n[ i%n.length ] );

int i;
for ( i = 0; i < 10; i++ ) {
v = stdlib_base_rising_factorial( x[ i ], n[ i ] );
printf( "x: %lf, n: %d, rising_factorial(x,n): %lf\n", x[ i ], n[ i ], v );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
printf( "x: %lf, n: %d, rising_factorial(x,n): %lf\n", x[ i ], n[ i ], v );
printf( "risingFactorial(%lf, %d) = %lf\n", x[ i ], n[ i ], v );

Following the JS equivalent example file.

*/

#include "stdlib/math/base/special/rising_factorial.h"
#include <stdlib.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <stdlib.h>
#include <stdint.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to be revisited using logEachMap


tape( 'the function returns `1` if provided `n = 0` and a nonnegative `x`', opts, function test( t ) {
var val = risingFactorial( 2.0, 0 );
t.equal( val, 1.0, 'returns expected value' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.equal( val, 1.0, 'returns expected value' );
t.strictEqual( val, 1.0, 'returns expected value' );

Applies here and elsewhere.


#### stdlib_base_rising_factorial( x, n )

Evaluates the rising factorial of `x` and `n`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Evaluates the rising factorial of `x` and `n`.
Evaluates the [rising factorial][falling-and-rising-factorials] of `x` and `n`.

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: passed
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: passed
  - task: lint_javascript_tests
    status: passed
  - task: lint_javascript_benchmarks
    status: passed
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: passed
  - task: lint_c_examples
    status: passed
  - task: lint_c_benchmarks
    status: passed
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
anandkaranubc
anandkaranubc previously approved these changes May 11, 2025
@Planeshifter Planeshifter added the Ready To Merge A pull request which is ready to be merged. label May 12, 2025
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label May 12, 2025
@Planeshifter Planeshifter merged commit c9777d9 into stdlib-js:develop May 12, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Math Issue or pull request specific to math functionality. Ready To Merge A pull request which is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants