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.

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: Added C implementation for @stdlib/math/base/special/fibonacci-index #2673

Closed

Conversation

vivek-anand-singh
Copy link

Resolves #1756

Description

What is the purpose of this pull request?

This pull request:

This adds the native C implementation for @stdlib/math/base/special/fibonacci-index.

double stdlib_base_fibonacci_index( const double F ); 

Related Issues

Does this pull request have any related issues?
No

This pull request:

Questions

Any questions for reviewers of this pull request?
I am not sure about the benchmark folder . Can you please guide over it.
I tried understanding the format from abs which was implemented inside @stdlib/math/base/special/abs .

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

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. C Issue involves or relates to C. labels Jul 27, 2024
@kgryte
Copy link
Member

kgryte commented Jul 27, 2024

@vivek-anand-singh This PR cannot be accepted as is.

  1. This PR also includes cusome-by. Please keep your PRs to only 1 feature. Multiple features should be split across separate PRs.
  2. You added @stdlib/math/special/fibonacci-index, and not @stdlib/math/base/special/fibonacci-index. Adding the former is not what was requested.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Jul 27, 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.

Please follow conventions elsewhere in the project. For submitting PRs, please see recent merged PRs for an idea of what is expected. Given that the intent was to add @stdlib/math/base/special/fibonacci-index, I suggest examining the PRs linked to #649.

@kgryte
Copy link
Member

kgryte commented Jul 27, 2024

As #1756 has already been implemented in #2272, I will go ahead and close this PR. @vivek-anand-singh I suggest finding another issue to work on, perhaps asking on #649 where work is being tracked.

@kgryte kgryte closed this Jul 27, 2024
@vivek-anand-singh
Copy link
Author

@kgryte, I deeply apologize for the mistakes in my PR.

For the first issue, I previously raised a PR on the cusome-by branch (PR #2661). When the second issue got assigned to me, I mistakenly assumed I was on the main branch and didn't switch branches. This oversight affected the PR, and I take full responsibility for it.

For the second issue, I overlooked the base folder, which I fully accept was my fault. I should have been more thorough in reviewing the project structure.

I sincerely apologize for any inconvenience caused. I genuinely want to learn from this experience and contribute positively to the organization in the future. I'm truly sorry for the oversight and would be grateful for another opportunity to make things right.

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. 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/fibonacci-index
2 participants