Skip to content

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

Merged
kgryte merged 20 commits intostdlib-js:developfrom
anandkaranubc:feat/factorial2f
Jun 1, 2025
Merged

feat: add math/base/special/factorial2f#6736
kgryte merged 20 commits intostdlib-js:developfrom
anandkaranubc:feat/factorial2f

Conversation

@anandkaranubc
Copy link
Copy Markdown
Contributor

@anandkaranubc anandkaranubc commented Apr 19, 2025

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:

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

@anandkaranubc
feat: temp commit
9e94d1f
@anandkaranubc
feat: temp commit
d91b4cb 
---
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
feat: temp commit
28419dc 
---
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
Merge branch 'develop' into feat/factorial2f
6dc82d7
@anandkaranubc
feat: add complete implementation
137d3e7 
---
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
---
@anandkaranubc
chore: update copyright years
1f5957b 
---
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
---
@stdlib-bot 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
anandkaranubc commented Apr 19, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/README.md
@stdlib-bot
Copy link
Copy Markdown
Contributor

stdlib-bot commented Apr 19, 2025 •
edited
Loading

Coverage Report

Package Statements Branches Functions Lines
math/base/special/factorial2f $\color{green}203/203$
$\color{green}+100.00\%$ 		$\color{green}19/19$
$\color{green}+100.00\%$ 		$\color{green}2/2$
$\color{green}+100.00\%$ 		$\color{green}203/203$
$\color{green}+100.00\%$

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

@anandkaranubc
docs: fix variable declaration
13b838b 
---
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
fix: convert the JS result to float32
a4cbab5 
---
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
---
@anandkaranubc 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
kgryte reviewed Apr 20, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c Outdated
@anandkaranubc
chore: apply suggestions from review
c14397f 
---
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
anandkaranubc commented Apr 20, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/test/fixtures/python/runner.py Outdated
@anandkaranubc
test: remove the approx check in JS tests
07f0a62 
---
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
Copy link
Copy Markdown
Contributor Author

anandkaranubc commented Apr 20, 2025

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

kgryte
kgryte reviewed Apr 20, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c
}
out = 1.0f;
for ( i = n; i >= last; i -= 2 ) {
out *= i;
Copy link
Copy Markdown
Member

@kgryte kgryte Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

kgryte
kgryte reviewed Apr 20, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/lib/main.js Outdated
@anandkaranubc
fix: convert the accumulated result to float32
c539eac 
---
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
---
@anandkaranubc
test: revert the changes to include approx tests in JS
d442f2e 
---
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 anandkaranubc marked this pull request as draft April 21, 2025 15:34
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label Apr 21, 2025
@anandkaranubc
Merge branch 'develop' into feat/factorial2f
8996360
@anandkaranubc
fix: correct imports to use max-nth-double-factorial constant
a4194cc 
---
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: 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 anandkaranubc marked this pull request as ready for review April 22, 2025 22:54
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Apr 22, 2025
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/README.md Outdated
@kgryte
docs: update desc
856cdbd 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/lib/index.js Outdated
@kgryte
docs: update desc
0e78fa0 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/package.json Outdated
@kgryte
docs: update desc
711080d 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/lib/main.js Outdated
@kgryte
refactor: simplify expressions
84eb082 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c Outdated
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c Outdated
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/src/main.c Outdated
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/test/test.js
kgryte
kgryte reviewed Apr 23, 2025
View reviewed changes
Comment thread lib/node_modules/@stdlib/math/base/special/factorial2f/test/fixtures/python/runner.py

def main():
"""Generate fixture data."""
x = np.arange(1, 57, dtype=np.int32)
Copy link
Copy Markdown
Member

@kgryte kgryte Apr 23, 2025

Choose a reason for hiding this comment

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

You may be able to adjust the precision here by using np.float32. np.int32 is likely to cause factorial2 to be evaluated in double-precision. NumPy is typically good about preserving float precision.

Copy link
Copy Markdown
Contributor Author

@anandkaranubc anandkaranubc Apr 23, 2025

Choose a reason for hiding this comment

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

Thanks @kgryte for the review, and sorry for the mistakes.

Also, I tried using np.float32, which requires the factorial2 function to be called with the additional parameter extend='complex'. As the documentation mentions, this changes the values of the double factorial for even integers by reducing them by a factor of sqrt(2/π). But even if we ignore that, the results for odd integers don't match perfectly either.

After a bit of head-twisting and surfing through the source code, I found that SciPy uses the gamma function to approximate the double factorial, rather than the straightforward iterative method we typically use. They have a wrapper function that generalizes the implementation for all types of factorials.

https://github.com/scipy/scipy/blob/0f1fd4a7268b813fa2b844ca6038e4dfdf90084a/scipy/special/_basic.py#L2992-L2997

Copy link
Copy Markdown
Contributor Author

@anandkaranubc anandkaranubc Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I could just generate the expected values locally using MATLAB.

Update: No luck with that :(

Copy link
Copy Markdown
Member

@kgryte kgryte Apr 23, 2025

Choose a reason for hiding this comment

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

No worries. Just add a note to the tests explaining why we are using approximate equality.

@anandkaranubc
fix: use correct imports and remove redundant checks
7d24f69 
---
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: 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
test: add comment on the use of approximate equality
1deff7c 
---
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 anandkaranubc deleted the feat/factorial2f branch May 10, 2025 14:35
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label May 10, 2025
@anandkaranubc anandkaranubc restored the feat/factorial2f branch May 14, 2025 08:42
@anandkaranubc anandkaranubc reopened this May 14, 2025
Planeshifter
Planeshifter approved these changes May 30, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Planeshifter Planeshifter left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All review comments have been addressed and the implementation looks good to me, so this PR should be ready to land.

@kgryte kgryte merged commit 4eaa59f into stdlib-js:develop Jun 1, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Planeshifter Planeshifter Planeshifter approved these changes

@kgryte kgryte kgryte approved these changes

Assignees

No one assigned

Labels

C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. JavaScript Issue involves or relates to JavaScript. Math Issue or pull request specific to math functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anandkaranubc @stdlib-bot @Planeshifter @kgryte