-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add constants/float16/gamma-lanczos-g
#9022
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
feat: add constants/float16/gamma-lanczos-g
#9022
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
lib/node_modules/@stdlib/constants/float16/gamma-lanczos-g/package.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Planeshifter
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.
Thanks!
PR Commit MessagePlease review the above commit message and make any necessary adjustments. |
|
@Amansingh0807 How did you derive the value of this constant? Do you have any sources you can share? |
The value added in #9022 is not appropriate for half-precision numerical approximation. As observed for the equivalent `float64` and `float32` constants, the value we use for `g` depends on what approximation we use in our gamma function implementations. In general, we should only add this constant if and when we add a half-precision gamma approximation. Something we are not likely to do anytime soon. BREAKING CHANGE: remove `constants/float16/lanczos-gamma-g` This feature was only recently added and has not been published. As such, the impact of removing this constant should be negligible for downstream users. Ref: #9022 --- 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: 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 ---
|
@Amansingh0807 After a bit of sanity checking, what you chose for the constant in this PR is a common value for approximating the gamma function in double-precision. That is not appropriate for half-precision. As observed for So, unfortunately, this PR should not have been merged, and I've reverted the addition in e2902df. For future reference, for some of these constants, they only make sense within the context of actual implementations, and, as we will not be adding a half-precision approximation of the gamma function anytime soon, this constant is not needed. Accordingly, it can be useful to first sanity check with maintainers to determine which constants, if any, we are interested in adding to the project. Hope this clarifies things, and sorry for the miscommunication. |
|
Thanks for the clarification @kgryte. This context helps a lot. From my side, I treated this similar to other constants that already exist across precisions and followed commonly used I understand that Thanks for explaining the rationale and reverting the change, the reasoning is clear now. I will make sure to check in advance for constants that are implementation-dependent rather than intrinsic. |
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves none
Description
This pull request:
constants/float16/gamma-lanczos-gRelated 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