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.

Sign up for GitHub

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

Jump to bottom

feat: add math/base/special/rempio2f #6142

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

feat: add math/base/special/rempio2f #6142

wants to merge 8 commits into from

Conversation

anandkaranubc
Copy link
Contributor

@anandkaranubc anandkaranubc commented Mar 18, 2025 •
edited
Loading

Progresses #649

Description

What is the purpose of this pull request?

This pull request:

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

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 and others added 3 commits March 11, 2025 12:33
@anandkaranubc
feat: temp commit
e5ddcf5
@anandkaranubc
feat: add implementation
66e8128 
---
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: 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: passed
  - task: lint_typescript_tests
    status: passed
  - task: lint_license_headers
    status: passed
---
@anandkaranubc
Merge branch 'stdlib-js:develop' into feat/rempio2f
2fe634a
@stdlib-bot stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Mar 18, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Mar 18, 2025 •
edited
Loading

Coverage Report

Package Statements Branches Functions Lines
math/base/special/rempio2f $\color{red}629/645$
$\color{green}+97.52\%$ 		$\color{red}45/51$
$\color{green}+88.24\%$ 		$\color{green}4/4$
$\color{green}+100.00\%$ 		$\color{red}629/645$
$\color{green}+97.52\%$

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

anandkaranubc
anandkaranubc commented Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/lib/main.js
*
* ## Notes
*
* - Returns `n` and stores the remainder `r` as `y[0]`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here comes a design choice: unlike rempio2, rempio2f returns only one value as the remainder. We can either stick with the current approach or change the return value to an array containing n and r.

Reference

Copy link
Member

Choose a reason for hiding this comment

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

This API matches rempio2, so that is fine.

@anandkaranubc
Copy link
Contributor Author

/stdlib update-copyright-years

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Mar 18, 2025
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Mar 18, 2025
anandkaranubc
anandkaranubc commented Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/examples/index.js
Comment on lines +29 to +32
for ( i = 0; i < x.length; i++ ) {
n = rempio2f( x[ i ], y );
console.log( '%d - %dπ/2 = %d', x[ i ], n, y[ 0 ] );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder how to use logEachMap here. Would y be an array of [ 0.0 ] arrays.

Copy link
Member

Choose a reason for hiding this comment

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

logEachMap doesn't work here, unfortunately.

anandkaranubc
anandkaranubc commented Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If needed, we can extract this function into a new package, kernel-rempio2.

The only difference from the kernel used in rempio2 is the precision-based case handling.

Reference

Copy link
Member

Choose a reason for hiding this comment

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

Yes, both kernel-rempio2 and kernel-rempio2f would be good to have as separate packages; although, they are much less likely to be used in the wild. Nevertheless, IMO, it would be cleaner. This, however, can be done at some later point as separate PRs, as it is not top priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Just a correction, there would still be one kernel.

The function signature would change from:

function kernelRempio2( x, y, e0, nx )

to:

function kernelRempio2( x, y, e0, nx, prec )

I'll add this to my notes for creating a future issue or updating #649.

kgryte
kgryte reviewed Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js Outdated
var j;
var m;

// Initialize `jk` for double-precision floating-point numbers:
Copy link
Member

Choose a reason for hiding this comment

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

Is this for double or single-precision floating-point numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the kernel is the almost the same as the one in rempio2.

kgryte
kgryte reviewed Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js
*
* @private
* @param {PositiveNumber} x - input value
* @param {(Array|TypedArray|Object)} y - output object for storing double precision numbers
Copy link
Member

Choose a reason for hiding this comment

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

Same question concerning double vs single precision.

kgryte
kgryte reviewed Mar 18, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js
*
* This table must have at least `(e0-3)/24 + jk` terms. For quad precision (`e0 <= 16360`, `jk = 6`), this is `686`.
*/
var IPIO2 = [
Copy link
Member

Choose a reason for hiding this comment

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

Why the difference of this implementation with what is in #6126?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for single precision, we don't need these many values. That's why it's not included in my code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this, I think you are right:

stdlib/lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js

Line 103 in c5a33c6

* @param {integer} jk - `jk+1` is the initial number of terms of `IPIO2[]` needed in the computation

Here, jk = 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is more clear:

stdlib/lib/node_modules/@stdlib/math/base/special/rempio2f/lib/kernel_rempio2f.js

Line 55 in c5a33c6

* This table must have at least `(e0-3)/24 + jk` terms. For quad precision (`e0 <= 16360`, `jk = 6`), this is `686`.

I will try to see the maximum value of e0 reached here. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering this, e0 = 127

So we need at least ~9 terms...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also aligns with the above comment on quad-precision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @kgryte

anandkaranubc and others added 4 commits March 18, 2025 00:15
@anandkaranubc
chore: apply suggestions from review
296b447 
---
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: 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
fix: initialisation of jk
c5a33c6 
---
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
Merge branch 'stdlib-js:develop' into feat/rempio2f
54ab542
@anandkaranubc
chore: fix comment for jk
1aed4e0 
---
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
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte kgryte left review comments

@dhruvilmehta dhruvilmehta dhruvilmehta left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anandkaranubc @stdlib-bot @kgryte @dhruvilmehta