-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add math/base/special/roundnf
#9389
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
base: develop
Are you sure you want to change the base?
feat: add math/base/special/roundnf
#9389
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
4a794b6 to
15255de
Compare
|
Hi @kgryte !! |
---
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: passed
- 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: 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: passed
- 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: 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: passed
- 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: 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: passed
- 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: 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: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
anandkaranubc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs a lot of cleanup. I think the best approach would be to go through all the comments above and the files again, and check whether it closely resembles what is being done in roundn. I have pushed a few changes, but there are still some remaining. Ping me whenever you need clarification on why certain comments and changes were made.
| * var v = roundnf( 3.141592, -2 ); | ||
| * // returns 3.140000104904175 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * var v = roundnf( 3.141592, -2 ); | |
| * // returns 3.140000104904175 | |
| * var v = roundnf( 3.1415927410125732, -2 ); | |
| * // returns ~3.14 |
It’s better to use approximate return values here using ~. If you use exact return values and the implementation changes for this package or the packages it depends on, it will break the linting for these files as well.
| * var roundnf = require( '@stdlib/math/base/special/roundnf' ); | ||
| * | ||
| * // If n = 0, `roundnf` behaves like `roundf`: | ||
| * var v = roundnf( 3.141592, 0 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * var v = roundnf( 3.141592, 0 ); | |
| * var v = roundnf( 3.1415927410125732, 0 ); |
Keeping the same example as above here too, for consistency.
| * @example | ||
| * var roundnf = require( '@stdlib/math/base/special/roundnf' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only need to use @example once in the index.js file. For more context you can refer to the index.js in roundn.
| var isnanf = require( '@stdlib/math/base/assert/is-nanf' ); | ||
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| var isnanf = require( '@stdlib/math/base/assert/is-nanf' ); | |
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | |
| var isnanf = require( '@stdlib/math/base/assert/is-nanf' ); |
You only need the single-precision based packages here.
| var roundf = require( '@stdlib/math/base/special/roundf' ); | ||
| var powf = require( '@stdlib/math/base/special/powf' ); | ||
| var absf = require( '@stdlib/math/base/special/absf' ); | ||
| var float64ToFloat32 = require( '@stdlib/number/float64/base/to-float32' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| var float64ToFloat32 = require( '@stdlib/number/float64/base/to-float32' ); | |
| var f32 = require( '@stdlib/number/float64/base/to-float32' ); |
| * @example | ||
| * var v = roundnf( 3.141592, -2 ); | ||
| * // returns 3.140000104904175 | ||
| * | ||
| * @example | ||
| * var v = roundnf( 3.141592, 0 ); | ||
| * // returns 3.0 | ||
| * | ||
| * @example | ||
| * var v = roundnf( 1234.56, 2 ); | ||
| * // returns 1200.0 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @example | |
| * var v = roundnf( 3.141592, -2 ); | |
| * // returns 3.140000104904175 | |
| * | |
| * @example | |
| * var v = roundnf( 3.141592, 0 ); | |
| * // returns 3.0 | |
| * | |
| * @example | |
| * var v = roundnf( 1234.56, 2 ); | |
| * // returns 1200.0 | |
| */ | |
| * @example | |
| * // Round a value to 2 decimal places: | |
| * var v = roundnf( 3.1415927410125732, -2 ); | |
| * // returns ~3.14 | |
| * | |
| * @example | |
| * // If n = 0, `roundnf` behaves like `roundf`: | |
| * var v = roundnf( 3.1415927410125732, 0 ); | |
| * // returns 3.0 | |
| * | |
| * @example | |
| * // Round a value to the nearest thousand: | |
| * var v = roundnf( 12368.0, 3 ); | |
| * // returns ~12000.0 | |
| */ |
You have to folllow the same example structure as main.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amansingh0807 This entire file needs revision. I would suggest reviewing https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/roundn/test/test.js. Copy over all the tests from that file and modify them accordingly for this package. It seems like you are missing several important tests.
Also, that file uses EPS-based thresholds. Can you use var ulpdiff = require( '@stdlib/number/float32/base/ulp-difference' ); here instead?
The same applies to test.native.js as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you refactor this as well, following what's done in https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/roundn/examples/index.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem the way we do C benchmarking. You will need to refer to what was done in https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/math/base/special/secf/benchmark/c/native/benchmark.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@Amansingh0807 I just finished reviewing #4881, and the expected changes here are very similar to the changes I recently made in that PR. The best way to update this PR would be to go through that one and follow the same commits. |
…mentation updates
|
Hi @anandkaranubc, |
Resolves None
Description
This pull request:
math/base/special/roundnf.Related Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
@stdlib-js/reviewers