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/minmax #3112

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

AbhijitRaut04
Copy link
Contributor

Resolves #2106 .

Description

This PR contains the C implementation for minmax function

This pull request:

  • C implementation with benchmarks added in this PR.

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 13, 2024
@AbhijitRaut04
Copy link
Contributor Author

@Planeshifter Can you please tell me why minmax.h is unable to locate ?

Comment on lines 33 to 41
"include": [
"./include"
],
"libraries": [],
"libpath": [],
"dependencies": [
"@stdlib/math/base/napi/unary",
"@stdlib/math/base/special/minmax"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

@AbhijitRaut04 You don't need to add the "@stdlib/math/base/special/minmax" in the manifest.json file.

Dependencies in manifest.json are for external packages or modules that the code relies on.

By listing ./include in the include path, the manifest.json specifies that any header files within that directory i.e. minmax.h are available.

And since, the C implementation of minmax is not yet implemented listing it in the dependencies is causing the CI errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@headlessNode Removed that dependencies but still not working

Copy link
Contributor

@headlessNode headlessNode Nov 14, 2024

Choose a reason for hiding this comment

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

have you run make install-node-addons NODE_ADDONS_PATTERN="math/base/special/minmax" command on your system?

These are compilation errors, if you run the above command on your system you can get a more verbose output to pinpoint the source of the errors. Hope it helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
running this command gives error

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure. This could be because of how your environment is configured.

@headlessNode
Copy link
Contributor

@AbhijitRaut04 There are some of inconsistencies in your PR. Namely, the STDLIB_MATH_BASE_NAPI_MODULE_D_D macro is specifically for registering a Node-API module that exports a unary function, one that accepts a single double precision input and returns a double precision output.

To see what macros are available for you see: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/math/base/napi

Also, from your lib folder you are missing the native.js file. You are also missing the documentation for the C in the README.

I suggest you go through the similar merged PRs to understand how to implement the C functionality, to start see: #3031

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Math Issue or pull request specific to math functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Add C implementation for @stdlib/math/base/special/minmax
5 participants