Skip to content
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

[RFC]: add C implementation for @stdlib/math/base/special/factorial2 #1892

Closed
3 tasks done
performant23 opened this issue Mar 14, 2024 · 5 comments · Fixed by #1898
Closed
3 tasks done

[RFC]: add C implementation for @stdlib/math/base/special/factorial2 #1892

performant23 opened this issue Mar 14, 2024 · 5 comments · Fixed by #1898
Assignees
Labels
C Issue involves or relates to C. difficulty: 2 May require some initial design or R&D, but should be straightforward to resolve and/or implement. Good First Issue A good first issue for new contributors! Math Issue or pull request specific to math functionality. priority: Normal Normal priority concern or feature request. RFC Request for comments. Feature requests and proposed changes.

Comments

@performant23
Copy link
Contributor

performant23 commented Mar 14, 2024

Description

This RFC proposes adding native C implementation for math/base/special/factorial2:

const double stdlib_base_factorial2( const double x);

Related Issues

#649

Questions

No.

Other

No.

Checklist

  • I have read and understood the Code of Conduct.
  • Searched for existing issues and pull requests.
  • The issue name begins with RFC:.
@performant23
Copy link
Contributor Author

Hello, working on this issue!

@performant23 performant23 changed the title [RFC]: add C implementation for @stdlib/math/base/special/factorial2 [RFC]: add C implementation for @stdlib/math/base/special/factorial2 Mar 14, 2024
@performant23
Copy link
Contributor Author

Also, just to clarify, in lib/main.js an example is given like this:

* var v = factorial2( 301 );
* // returns Infinity

Also in fixtures/integers.json, the factorials are defined from 0-300 inclusive. So since factorial2 for 301 isn't defined, won't it be invalid to use 301?
@Pranavchiku, @kgryte, @Planeshifter

var MAX_FACTORIAL2 = 301; // TODO: consider extracting as a constant
.
.
.
if ( n > MAX_FACTORIAL2 ) {
return PINF;
}

@kgryte kgryte added RFC Request for comments. Feature requests and proposed changes. Math Issue or pull request specific to math functionality. Accepted RFC feature request which has been accepted. Good First Issue A good first issue for new contributors! priority: Normal Normal priority concern or feature request. C Issue involves or relates to C. difficulty: 2 May require some initial design or R&D, but should be straightforward to resolve and/or implement. labels Mar 15, 2024
@kgryte
Copy link
Member

kgryte commented Mar 15, 2024

I suggest implementing similar to the C implementation in fibonacci, where the C implementation accepts an integer and returns a double.

@kgryte kgryte removed the Accepted RFC feature request which has been accepted. label Mar 15, 2024
@performant23
Copy link
Contributor Author

Got it, will do! Thanks, @kgryte! Just had a little confusion about the return value of the double factorial for 301.
Since we bound the input to 301 as MAX_FACTORIAL2, and use the comparison as above effectively returning infinity for n > 301, we are accepting 301 exactly as an input for further processing.

But the example shown above suggests that we return infinity for 301.

So, if we want to return infinity for 301, we would need something like n >= 301 or set MAX_FACTORIAL2 to 300 is what I thought, or am I missing something?

@kgryte
Copy link
Member

kgryte commented Mar 15, 2024

The documentation is actually correct.

In [42]: base.factorial2(301)
Out[42]: Infinity

301 actually computes to infinity. So, the implementation could be updated to use greater than or equal to rather than >. Feel free to make that small tweak when adding the C implementation.

Planeshifter added a commit that referenced this issue Mar 15, 2024
PR-URL: #1898 
Closes: #1892

---------

Signed-off-by: Rutam <138517416+performant23@users.noreply.github.com>
Signed-off-by: Pranav <85227306+Pranavchiku@users.noreply.github.com>
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Co-authored-by: Pranav <85227306+Pranavchiku@users.noreply.github.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Reviewed-by: Pranav <85227306+Pranavchiku@users.noreply.github.com>
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Issue involves or relates to C. difficulty: 2 May require some initial design or R&D, but should be straightforward to resolve and/or implement. Good First Issue A good first issue for new contributors! Math Issue or pull request specific to math functionality. priority: Normal Normal priority concern or feature request. RFC Request for comments. Feature requests and proposed changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants