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/acosdf #3015

Merged
merged 18 commits into from
Oct 29, 2024
Merged

feat: add math/base/special/acosdf #3015

merged 18 commits into from
Oct 29, 2024

Conversation

aayush0325
Copy link
Contributor

@aayush0325 aayush0325 commented Oct 17, 2024 •
edited by kgryte
Loading

Resolves part of #649.

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

Open to reviews!!

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 17, 2024
@aayush0325
Copy link
Contributor Author

aayush0325 commented Oct 17, 2024 •
edited
Loading

@gunjjoshi would love your inputs on this, is it okay if i had to increase the tolerance in the tests a bit to accommodate single-precision floating point numbers?

@gunjjoshi
Copy link
Member

@gunjjoshi would love your inputs on this, is it okay if i had to increase the tolerance in the tests a bit to accommodate single-precision floating point numbers?

I think the tolerance levels that we have here, i.e., 2.8 * EPS should be fine, as compared to the 1.4 * EPS of the double-precision equivalent, as we can expect a slight precision loss when we transition from double to single-precision floating-point numbers.

@Planeshifter Planeshifter added the Needs Review A pull request which needs code review. label Oct 17, 2024
@aayush0325
Copy link
Contributor Author

that's what i thought while increasing the tolerance, do you think there's any other stuff that needs to be done in order to merge this @gunjjoshi , would love to work on this

@kgryte kgryte requested a review from gunjjoshi October 17, 2024 20:08
@kgryte kgryte added Feature Issue or pull request for adding a new feature. C Issue involves or relates to C. labels Oct 17, 2024
@kgryte kgryte changed the title feat: add C implementation for math/base/special/acosdf feat: add math/base/special/acosdf Oct 17, 2024
@kgryte kgryte removed the C Issue involves or relates to C. label Oct 17, 2024
@aayush0325
Copy link
Contributor Author

added the changes @gunjjoshi

@kgryte
Copy link
Member

kgryte commented Oct 26, 2024

@gunjjoshi Would you mind doing one more pass over this PR?

@gunjjoshi
Copy link
Member

Only a single change needed, looks good otherwise.

@kgryte @gunjjoshi
Apply suggestions from code review
ee4d4e3 
Co-authored-by: Gunj Joshi <[email protected]>
Signed-off-by: Athan <[email protected]>
@aayush0325
Copy link
Contributor Author

is there anything that is left to do here @gunjjoshi ? , would be happy to implement

Planeshifter
Planeshifter approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Did a review pass and left one more suggestion. Otherwise, this looks ready to merge for me. Thanks for your contribution!

lib/node_modules/@stdlib/math/base/special/acosdf/README.md Outdated Show resolved Hide resolved
@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. labels Oct 29, 2024
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Oct 29, 2024 •
edited by Planeshifter
Loading

PR Commit Message

feat: add `math/base/special/acosdf`

PR-URL: https://github.com/stdlib-js/stdlib/pull/3015
Ref: https://github.com/stdlib-js/stdlib/issues/649

Co-authored-by: Athan Reines <[email protected]>
Signed-off-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]> 
Reviewed-by: Gunj Joshi <[email protected]>
Reviewed-by: Athan Reines <[email protected]>

Please review the above commit message and make any necessary adjustments.

@Planeshifter Planeshifter merged commit b18921a into stdlib-js:develop Oct 29, 2024
18 checks passed
@aayush0325 aayush0325 deleted the acosdf branch October 29, 2024 14:22
Neerajpathak07 pushed a commit to Neerajpathak07/stdlib that referenced this pull request Nov 9, 2024
@aayush0325 @kgryte @Neerajpathak07
feat: add math/base/special/acosdf
526e0e1 
PR-URL: stdlib-js#3015
Ref: stdlib-js#649

Co-authored-by: Athan Reines <[email protected]>
Signed-off-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]> 
Reviewed-by: Gunj Joshi <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
aayush0325 added a commit to aayush0325/stdlib that referenced this pull request Nov 11, 2024
@aayush0325 @kgryte
feat: add math/base/special/acosdf
26a72bf 
PR-URL: stdlib-js#3015
Ref: stdlib-js#649

Co-authored-by: Athan Reines <[email protected]>
Signed-off-by: Athan Reines <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]> 
Reviewed-by: Gunj Joshi <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
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

@gunjjoshi gunjjoshi gunjjoshi approved these changes

@Planeshifter Planeshifter Planeshifter approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aayush0325 @gunjjoshi @kgryte @stdlib-bot @Planeshifter