Skip to content

feat: add C implementation for math/base/special/sin #2031

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

Closed
wants to merge 1 commit into from

Conversation

xaman27x
Copy link

Description

What is the purpose of this pull request?

This pull request:
Adds native C implementation for @stdlib/math/base/special/sin
double stdlib_base_sin ( const double x );

Related Issues

Issue Tracker: #2018

This pull request:

  1. Adds C Implementation of @stdlib/node_moudles/math/base/special/sin

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

Signed-off-by: Aman Morghade <136126298+xaman27x@users.noreply.github.com>
@xaman27x xaman27x marked this pull request as ready for review March 25, 2024 07:33
@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Mar 25, 2024
@xaman27x xaman27x changed the title Add files via upload Adding C implementation for @stdlib/node_moudles/math/base/special/sin Mar 25, 2024
@xaman27x
Copy link
Author

NOTE:
The sin function contains precision upto 4 floating points. (ie. +-0.00001)
Thanks!

@xaman27x xaman27x changed the title Adding C implementation for @stdlib/node_moudles/math/base/special/sin Adding C implementation for @stdlib/node_modules/math/base/special/sin Mar 25, 2024
@Pranavchiku Pranavchiku changed the title Adding C implementation for @stdlib/node_modules/math/base/special/sin feat: add C implementation for @stdlib/node_modules/math/base/special/sin Mar 25, 2024
@Pranavchiku Pranavchiku changed the title feat: add C implementation for @stdlib/node_modules/math/base/special/sin feat: add C implementation for math/base/special/sin Mar 25, 2024
Copy link
Member

@Pranavchiku Pranavchiku left a comment

Choose a reason for hiding this comment

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

This PR is missing files, please refer similar PRs add those and then this can be reviewed. Also sin implementation depends on rempio2 for which we do not have C implementation atm, so this PR is blocked.

@Pranavchiku Pranavchiku added Feature Issue or pull request for adding a new feature. Native Addons Issue involves or relates to Node.js native add-ons. status: Blocked Issue or pull request which is currently blocked. C Issue involves or relates to C. Needs Changes Pull request which needs changes before being merged. labels Mar 25, 2024
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@xaman27x As is, we cannot accept the changes introduced in this PR. Please ensure you study how other packages include C implementations.

E.g., don't write your own binding.gyp file, copy it from other packages, comments and all.

You shouldn't be modifying the contents of package.json, except for gypfile and directories. Use project Makefiles, copying them from other packages.

We have project tooling for compilation. Learn that tooling.

Your addon interface does not conform to our conventions.

Your C implementation needs considerable work. sin depends on rempio2: https://svnweb.freebsd.org/base/release/9.3.0/lib/msun/src/s_sin.c?view=markup. Meaning, the prerequisite for adding a C implementation to sin has not been satisfied in order to add this implementation.

@xaman27x
Copy link
Author

@xaman27x As is, we cannot accept the changes introduced in this PR. Please ensure you study how other packages include C implementations.

E.g., don't write your own binding.gyp file, copy it from other packages, comments and all.

You shouldn't be modifying the contents of package.json, except for gypfile and directories. Use project Makefiles, copying them from other packages.

We have project tooling for compilation. Learn that tooling.

Your addon interface does not conform to our conventions.

Your C implementation needs considerable work. sin depends on rempio2: https://svnweb.freebsd.org/base/release/9.3.0/lib/msun/src/s_sin.c?view=markup. Meaning, the prerequisite for adding a C implementation to sin has not been satisfied in order to add this implementation.

Alright! I'll copy all necessary files henceforth! And since the Sin implementation requires C implementation of 'rempio2' as well, I'll work upon that first!
Thank you for consideration
Best.

@kgryte
Copy link
Member

kgryte commented Jun 11, 2024

This PR has been superseded by #2349. Will go ahead and close. Thank you, @xaman27x, for your interest in stdlib.

@kgryte kgryte added the Obsolete Issue or pull request which is no longer relevant. label Jun 11, 2024
@kgryte kgryte closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. Native Addons Issue involves or relates to Node.js native add-ons. Needs Changes Pull request which needs changes before being merged. Obsolete Issue or pull request which is no longer relevant. status: Blocked Issue or pull request which is currently blocked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants