feat: add `math/base/special/factorial2f` #6736

anandkaranubc · 2025-04-19T23:51:12Z

Progresses #649

Description

What is the purpose of this pull request?

This pull request:

Adds the JS and C implementation of math/base/special/factorial2f.

Related Issues

Does this pull request have any related issues?

This pull request:

progresses [RFC]: Add C implementations to base special math functions (tracking issue) #649

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.

Read, understood, and followed the contributing guidelines.

@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: na - 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: passed - 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_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: passed - 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: passed - 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: passed - task: lint_typescript_tests status: passed - task: lint_license_headers status: passed ---

--- 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: na - task: lint_python status: na - task: lint_r status: na - task: lint_c_src status: passed - 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 ---

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

stdlib-bot · 2025-04-19T23:53:00Z

Coverage Report

Package	Statements	Branches	Functions	Lines
math/base/special/factorial2f	$\color{green}204/204$ $\color{green}+100.00\%$	$\color{green}19/19$ $\color{green}+100.00\%$	$\color{green}2/2$ $\color{green}+100.00\%$	$\color{green}204/204$ $\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 ---

anandkaranubc · 2025-04-19T23:58:35Z

lib/node_modules/@stdlib/math/base/special/factorial2f/test/test.native.js

+			t.strictEqual( v, expected[ i ], 'returns expected value' );
+		} else {
+			delta = absf( v - expected[i] );
+			tol = 2.4 * EPS * absf( expected[i] );


It’s still not entirely clear to me why it’s higher here. I checked, and it’s not due to CFLAGS. Seems more like a floating-point arithmetic difference in C v/s JS implementation.

Yeah, this doesn't make sense. Something seems off. We are multiplying integers. There shouldn't be precision loss. I am not even sure why we have approx tests in the JS version in the first place.

In fact, in the original commit, there weren't approx tests: c0dab03

Okay. I see. You've added approx tests for the single-precision versions. Not sure why that is necessary.

Oh, right. Your fixtures are evaluated in double-precision. You need to rethink your fixture generation strategy.

In fact, in the original commit, there weren't approx tests: c0dab03

The only reason I used fixture generation instead of simply copying and truncating the JSON from factorial2 was because those values were evaluated in double precision.

On a side note, I also tried using the same JSON from factorial2 and casting the expected values to float32 using float64toFloat32 before comparing them to the calculated values. With this change, there's no need for approximate checks in the JS tests, but the native tests still use the same tolerance value of 2.4 * EPS.

So the same scenario as what we have right now :(

The reason for the discrepancy between JS and C is that the intermediate accumulation is happening in different precisions. Currently, in JS, the accumulation is happening in double. In C, the accumulation is happening in float.

If you want to see the same results in JS, you need to cast each intermediate accumulation via float64ToFloat32.

🙇🏻‍♂️

--- 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: na - 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: 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 ---

kgryte · 2025-04-20T06:26:22Z

lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c

+	if ( n < 0 || !stdlib_base_is_integerf( n ) ) {
+		return 0.0f / 0.0f; // NaN
+	}
+	v = n;


v is not necessary. We do that in JS in order to cast the implicit double input argument to int32 using an asm type annotation.

--- 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: 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: passed - 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 ---

anandkaranubc · 2025-04-20T08:14:21Z

lib/node_modules/@stdlib/math/base/special/factorial2f/test/fixtures/python/runner.py

+    python> gen(x, './data.json')
+    ```
+    """
+    y = factorial2(x, exact=False)


Suggested change

y = factorial2(x, exact=False)

y = factorial2(x, exact=False).astype(np.float32)

However, this change still doesn't affect the return values of the test fixtures.

--- 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: na - 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: passed - 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 ---

anandkaranubc · 2025-04-20T08:19:30Z

lib/node_modules/@stdlib/math/base/special/factorial2f/test/test.native.js

+	expected = data.expected;
+
+	for ( i = 0; i < x.length; i++ ) {
+		v = factorial2f( x[ i ] );


Suggested change

v = factorial2f( x[ i ] );

v = factorial2f( x[ i ] );

expected[ i ] = float64toFloat32( expected[ i ] );

No change in tol with this update either.

anandkaranubc · 2025-04-20T08:31:50Z

@kgryte Sorry if my recent comments cause any confusion. Just wanted to summarize everything I found so far.

kgryte · 2025-04-20T08:34:23Z

lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c

+	}
+	out = 1.0f;
+	for ( i = n; i >= last; i -= 2 ) {
+		out *= i;


In C, this is equivalent to out = out * (float)i;.

kgryte · 2025-04-20T08:35:13Z

lib/node_modules/@stdlib/math/base/special/factorial2f/lib/main.js

+	}
+	out = 1;
+	for ( i = v|0; i >= last; i -= 2|0 ) {
+		out *= i|0; // asm type annotation


In JS, this is equivalent to out = out * (double)i;.

--- 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: na - 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: 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_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: na - 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: passed - task: lint_javascript_benchmarks status: na - task: lint_python status: passed - 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 ---

anandkaranubc added 6 commits March 29, 2025 14:31

feat: temp commit

9e94d1f

Merge branch 'develop' into feat/factorial2f

6dc82d7

stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Apr 19, 2025

anandkaranubc commented Apr 19, 2025

View reviewed changes

lib/node_modules/@stdlib/math/base/special/factorial2f/README.md Outdated Show resolved Hide resolved

anandkaranubc commented Apr 19, 2025

View reviewed changes

anandkaranubc added Feature Issue or pull request for adding a new feature. JavaScript Issue involves or relates to JavaScript. C Issue involves or relates to C. labels Apr 20, 2025

kgryte reviewed Apr 20, 2025

View reviewed changes

anandkaranubc commented Apr 20, 2025

View reviewed changes

kgryte reviewed Apr 20, 2025

View reviewed changes

anandkaranubc added 2 commits April 20, 2025 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `math/base/special/factorial2f` #6736

feat: add `math/base/special/factorial2f` #6736

anandkaranubc commented Apr 19, 2025

stdlib-bot commented Apr 19, 2025 •

edited

Loading

anandkaranubc Apr 19, 2025

kgryte Apr 20, 2025

kgryte Apr 20, 2025

kgryte Apr 20, 2025

kgryte Apr 20, 2025

anandkaranubc Apr 20, 2025

anandkaranubc Apr 20, 2025

kgryte Apr 20, 2025

kgryte Apr 20, 2025

anandkaranubc Apr 20, 2025

kgryte Apr 20, 2025

anandkaranubc Apr 20, 2025

anandkaranubc Apr 20, 2025

anandkaranubc commented Apr 20, 2025

kgryte Apr 20, 2025 •

edited

Loading

kgryte Apr 20, 2025

	y = factorial2(x, exact=False)
	y = factorial2(x, exact=False).astype(np.float32)

	v = factorial2f( x[ i ] );
	v = factorial2f( x[ i ] );
	expected[ i ] = float64toFloat32( expected[ i ] );

feat: add math/base/special/factorial2f #6736

Are you sure you want to change the base?

feat: add math/base/special/factorial2f #6736

Conversation

anandkaranubc commented Apr 19, 2025

Description

Related Issues

Questions

Other

Checklist

stdlib-bot commented Apr 19, 2025 • edited Loading

Coverage Report

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

anandkaranubc commented Apr 20, 2025

kgryte Apr 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `math/base/special/factorial2f` #6736

feat: add `math/base/special/factorial2f` #6736

stdlib-bot commented Apr 19, 2025 •

edited

Loading

kgryte Apr 20, 2025 •

edited

Loading