Skip to content

feat: add math/base/special/bernoullif #3037

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
merged 21 commits into from
May 12, 2025

Conversation

gururaj1512
Copy link
Member

@gururaj1512 gururaj1512 commented Oct 26, 2024

Progresses #649

Description

What is the purpose of this pull request?

float stdlib_base_bernoullif( 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

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Oct 26, 2024
@@ -0,0 +1,34 @@
[
1.00000000000000000000000000000000000000000,
Copy link
Member

Choose a reason for hiding this comment

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

These values all have very high precision. Is there a reason for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I have used similar values as of math/base/special/bernoulli. Should I make it less precise?

Copy link
Member

@gunjjoshi gunjjoshi Apr 3, 2025

Choose a reason for hiding this comment

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

No, I have used similar values as of math/base/special/bernoulli. Should I make it less precise?

Yes, @gururaj1512.

You'll have to keep these values in appropriate precision. You can use https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/number/float64/base/to-float32 for the conversion.

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. labels Oct 26, 2024
gururaj1512 and others added 2 commits October 27, 2024 20:32
Co-authored-by: Athan <kgryte@gmail.com>
Signed-off-by: Gururaj Gurram <143020143+gururaj1512@users.noreply.github.com>
@Planeshifter
Copy link
Member

@gunjjoshi This is not a problem exclusively with this PR, since it also applies to math/base/special/bernoulli, which you added in #1860, but I noticed that the implementation returns zero for all odd numbers. However, Wikipedia says that this is incorrect for input value 1: https://en.wikipedia.org/wiki/Bernoulli_number
Could you please take a look and confirm whether we need to handle this case separately?

@gunjjoshi
Copy link
Member

@Planeshifter Thanks for catching this. After digging for some time, I found that there is a mix of opinions on whether B(1) = +1/2 or B(1) = -1/2. For instance, here is a discussion from sympy: sympy/sympy#23866.
While it is incorrect to use B(1) = 0, the choice that must be made is between +1/2 or -1/2.
sympy now uses +1/2 (it used -1/2 earlier). There's a discussion at sagemath too: sagemath/sage#34521.

Overall, I think, it would be better if we too, use B(1) = +1/2. I'll make this change in math/base/special/bernoulli, if this is fine.

@gunjjoshi
Copy link
Member

gunjjoshi commented Nov 11, 2024

But yes, NIST prescribes the convention to use B(1) = -1/2: https://en.wikipedia.org/wiki/Bernoulli_number#Notation
What should we follow, in this case?

@kgryte
Copy link
Member

kgryte commented Nov 13, 2024

Let's go ahead and use 1/2. This will match SymPy and is also used as a convention in R. As commented elsewhere, how much the choice actually matters in practice is subject to some debate. In this case, I'm more inclined to go with Knuth.

@kgryte
Copy link
Member

kgryte commented Nov 13, 2024

Similar to the changes proposed in #3108, we'll want to make similar changes here.

kgryte added a commit that referenced this pull request Nov 14, 2024
…lli`

BREAKING CHANGE: update return value for `n=1`

In order to migrate and preserve prior behavior, users should special case `n=1` and return `0`. The change in this commit aligns return values with SymPy and R; although, other libraries and envs choose to return `-0.5`.

PR-URL: #3108
Ref: #3037 (comment)
Co-authored-by: Athan Reines <kgryte@gmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com> 
Signed-off-by: Gunj Joshi <gunjjoshi8372@gmail.com>
Signed-off-by: Athan Reines <kgryte@gmail.com>
@gururaj1512
Copy link
Member Author

Refactored it as suggested.

@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

/stdlib merge

@kgryte kgryte added the Needs Review A pull request which needs code review. label Nov 18, 2024
@kgryte kgryte requested a review from gunjjoshi November 18, 2024 04:12

// MODULES //

var isNonNegativeInteger = require( '@stdlib/math/base/assert/is-nonnegative-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
var isNonNegativeInteger = require( '@stdlib/math/base/assert/is-nonnegative-integer' );
var isNonNegativeIntegerf = require( '@stdlib/math/base/assert/is-nonnegative-integerf' );

return 0.0;
}
if ( n > MAX_BERNOULLI ) {
return ( ( n / 2 ) & 1 ) ? PINF : NINF;
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 ( ( n / 2 ) & 1 ) ? PINF : NINF;
return ( (n/2)&1 ) ? PINF : NINF;

Comment on lines 20 to 29

var bernoullif = require( './../lib' );

var v;
var i;

for ( i = 0; i < 70; i++ ) {
v = bernoullif( i );
console.log( 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
var bernoullif = require( './../lib' );
var v;
var i;
for ( i = 0; i < 70; i++ ) {
v = bernoullif( i );
console.log( v );
}
var logEachMap = require( '@stdlib/console/log-each-map' );
var discreteUniform = require( '@stdlib/random/array/discrete-uniform' );
var bernoullif = require( './../lib' );
var x = discreteUniform( 100, 0, 70, {
'dtype': 'int32'
});
logEachMap( 'bernoullif(%d) = %0.4f', x, bernoullif );


for ( i = 0; i < 70; i++ ) {
v = stdlib_base_bernoullif( i );
printf( "bernoulli(%d) = %f\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( "bernoulli(%d) = %f\n", i, v );
printf( "bernoullif(%d) = %f\n", i, v );


for ( i = -1; i > -50; i-- ) {
v = bernoullif( i );
t.strictEqual( isnanf( v ), true, 'returns expected value when provided ' + i );
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.strictEqual( isnanf( v ), true, 'returns expected value when provided ' + i );
t.strictEqual( isnanf( v ), true, 'returns expected value' );

Applies here and below...

tape( 'the function returns the nth Bernoulli number for odd numbers', function test( t ) {
var v;
var i;
for ( i = 3; i < 500; i += 2 ) {
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
for ( i = 3; i < 500; i += 2 ) {
for ( i = 3; i < 100; i += 2 ) {

A smaller range is ok for bernoullif

tape( 'the function returns +/- infinity for large integers', function test( t ) {
var v;
var i;
for ( i = 66; i < 500; i += 2 ) {
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
for ( i = 66; i < 500; i += 2 ) {
for ( i = 66; i < 150; i += 2 ) {

Same comment.

// MODULES //

var bench = require( '@stdlib/bench' );
var randu = require( '@stdlib/random/array/discrete-uniform' );
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
var randu = require( '@stdlib/random/array/discrete-uniform' );
var discreteUniform = require( '@stdlib/random/array/discrete-uniform' );

For being consistent across other packages.

var y;
var i;

x = randu( 100, 0, 500 );
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
x = randu( 100, 0, 500 );
x = discreteUniform( 100, 0, 100 );

100 is fine here for the max value.

int i;

for ( i = 0; i < 100; i++ ) {
x[ i ] = ( 500.0 * rand_float() );
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
x[ i ] = ( 500.0 * rand_float() );
x[ i ] = ( 500.0f * rand_float() );


t = tic();
for ( i = 0; i < ITERATIONS; i++ ) {
y = stdlib_base_bernoullif( (int)( x[ 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_bernoullif( (int)( x[ i % 100 ] ) );
y = stdlib_base_bernoullif( (int32_t)( x[ i%100 ] ) );

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

Choose a reason for hiding this comment

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

@stdlib/math/base/assert/is-odd is not needed here now.

"stdmath",
"mathematics",
"math",
"special functions",
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
"special functions",

Keywords with more than one word are not used.

Comment on lines 111 to 119
var bernoullif = require( '@stdlib/math/base/special/bernoullif' );

var v;
var i;

for ( i = 0; i < 70; i++ ) {
v = bernoullif( i );
console.log( 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
var bernoullif = require( '@stdlib/math/base/special/bernoullif' );
var v;
var i;
for ( i = 0; i < 70; i++ ) {
v = bernoullif( i );
console.log( v );
}
var logEachMap = require( '@stdlib/console/log-each-map' );
var discreteUniform = require( '@stdlib/random/array/discrete-uniform' );
var bernoullif = require( './../lib' );
var x = discreteUniform( 100, 0, 70, {
'dtype': 'int32'
});
logEachMap( 'bernoullif(%d) = %0.4f', x, bernoullif );


```c
float out = stdlib_base_bernoullif( 0 );
// returns 1.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
// returns 1.0
// returns 1.0f

// returns 1.0

out = stdlib_base_bernoullif( 1 );
// returns 0.5
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
// returns 0.5
// returns 0.5f

---
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
---
anandkaranubc
anandkaranubc previously approved these changes May 12, 2025
Planeshifter
Planeshifter previously approved these changes May 12, 2025
@Planeshifter Planeshifter added Ready To Merge A pull request which is ready to be merged. and removed Needs Review A pull request which needs code review. Needs Changes Pull request which needs changes before being merged. labels May 12, 2025
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter Planeshifter dismissed stale reviews from anandkaranubc and themself via dc31e82 May 12, 2025 10:29
@Planeshifter Planeshifter added Ready To Merge A pull request which is ready to be merged. and removed Ready To Merge A pull request which is ready to be merged. labels May 12, 2025
@Planeshifter Planeshifter merged commit 7d671b1 into stdlib-js:develop May 12, 2025
28 of 29 checks passed
@gururaj1512 gururaj1512 deleted the bernoullif branch May 12, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. 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.

6 participants