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: add C implementation for math/base/special/dirichlet-eta #3282

Merged
merged 7 commits into from
Dec 7, 2024

Conversation

gunjjoshi
Copy link
Member

Progresses #649.

Description

What is the purpose of this pull request?

This pull request:

double stdlib_base_eta( const double s )

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.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Nov 27, 2024
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Nov 27, 2024

Coverage Report

Package Statements Branches Functions Lines
math/base/special/dirichlet-eta $\color{green}185/185$
$\color{green}+100.00\%$
$\color{green}9/9$
$\color{green}+100.00\%$
$\color{green}2/2$
$\color{green}+100.00\%$
$\color{green}185/185$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

@gunjjoshi
Copy link
Member Author

We get the same lint error here as well, that came at: https://github.com/stdlib-js/stdlib/actions/runs/12022789799/job/33515586260#step:17:1332

@gunjjoshi
Copy link
Member Author

It seems that this might be due to different function names, such as we use dirichlet_eta.h for the header file, and stdlib_base_eta, instead of stdlib_base_dirichlet_eta for the main function. The same error is encountered in riemann-zeta, where we use riemann_zeta.h and stdlib_base_zeta.

@gunjjoshi
Copy link
Member Author

It seems that this might be due to different function names, such as we use dirichlet_eta.h for the header file, and stdlib_base_eta, instead of stdlib_base_dirichlet_eta for the main function. The same error is encountered in riemann-zeta, where we use riemann_zeta.h and stdlib_base_zeta.

Okay, I tried changing the C implementation to use stdlib_base_dirichlet_eta instead of stdlib_base_eta, but that didn't work.

Signed-off-by: Gunj Joshi <[email protected]>

Choose a reason for hiding this comment

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

Copilot reviewed 21 out of 21 changed files in this pull request and generated no suggestions.

@Planeshifter
Copy link
Member

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Dec 1, 2024
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Dec 1, 2024
@kgryte kgryte added Needs Review A pull request which needs code review. C Issue involves or relates to C. labels Dec 4, 2024
@Planeshifter
Copy link
Member

Planeshifter commented Dec 7, 2024

@gunjjoshi Encountered the same issue in a different PR. Problem should go away after updating the package.json file.

@stdlib/_tools/pkgs/includes, which is used to generate the includes list used by cppcheck via our make recipes, expects the include directory to be set in the package.json directories:

  "gypfile": true,
  "directories": {
    "benchmark": "./benchmark",
    "doc": "./docs",
    "example": "./examples",
    "include": "./include",
    "lib": "./lib",
    "scripts": "./scripts",
    "src": "./src",
    "test": "./test"
  },

We should add a lint check for whether "directories" and gypfile is properly populated in the package.json and raise an error if not. This could be done by invoking the same scripts as in https://github.com/stdlib-js/stdlib/blob/develop/.github/workflows/update_package_meta_data.yml and then simply erroring in case there are changes (could check e.g. via if [[ git status --porcelain ]]; then).

cc @kgryte

@Planeshifter
Copy link
Member

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Dec 7, 2024
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Dec 7, 2024
@Planeshifter
Copy link
Member

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Dec 7, 2024
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Dec 7, 2024
@gunjjoshi
Copy link
Member Author

@Planeshifter That worked. Thanks!

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!

@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 Dec 7, 2024
@Planeshifter Planeshifter merged commit ef06a2d into stdlib-js:develop Dec 7, 2024
18 of 19 checks passed
@kgryte
Copy link
Member

kgryte commented Dec 10, 2024

@stdlib/_tools/pkgs/includes, which is used to generate the includes list used by cppcheck via our make recipes, expects the include directory to be set in the package.json

Thanks for tracking this down, @Planeshifter! And agreed. Thanks for setting up linting to catch this in the future.

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. Math Issue or pull request specific to math functionality. Ready To Merge A pull request which is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants