Skip to content

feat: add C implementation for math/base/special/ahaversin #1724

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 7 commits into from
Closed

feat: add C implementation for math/base/special/ahaversin #1724

wants to merge 7 commits into from

Conversation

kawaho2
Copy link
Contributor

@kawaho2 kawaho2 commented Mar 6, 2024

Resolves #1420 .

Description

What is the purpose of this pull request?

This pull request:Add C implementation for @stdlib/math/base/special/ahaversin

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.

  • Read, understood, and followed the [contributing guidelines][contributing].

@stdlib-js/reviewers

@Planeshifter Planeshifter changed the title feat: add C implementation for @stdlib/math/base/special/ahaversin feat: add C implementation for math/base/special/ahaversin Mar 6, 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.

Going good, requested a few changes. Thanks @Rejoan-Sardar !

@Pranavchiku Pranavchiku added 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. C Issue involves or relates to C. Needs Changes Pull request which needs changes before being merged. labels Mar 6, 2024
@kawaho2 kawaho2 requested a review from Pranavchiku March 6, 2024 20:33
@kawaho2
Copy link
Contributor Author

kawaho2 commented Mar 9, 2024

@Pranavchiku I have addressed all the issues you mentioned in your review. Please take a look at the 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.

LGTM. Much appreciated, @Rejoan-Sardar!

@kawaho2
Copy link
Contributor Author

kawaho2 commented Mar 10, 2024

LGTM. Much appreciated, @Rejoan-Sardar!

Great, thanks for that.

@kawaho2
Copy link
Contributor Author

kawaho2 commented Mar 11, 2024

LGTM. Much appreciated, @Rejoan-Sardar!

@Planeshifter sir u approved these changes and said that LGTM. Much appreciated but not merged this PR.Sir any changes need from my side.

@Planeshifter
Copy link
Member

@Rejoan-Sardar Looks like CI is failing and I didn't find the time to properly debug. I think there is a problem running benchmarks and examples.

LIBPATH ?=

# List of C targets:
c_targets := benchmark.out
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
c_targets := benchmark.out
c_targets := example.out

Signed-off-by: Rejoan Sardar <[email protected]>
@kawaho2 kawaho2 closed this Mar 12, 2024
@kawaho2 kawaho2 deleted the ahaversin branch March 12, 2024 10:42
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add C implementation for @stdlib/math/base/special/ahaversin
3 participants