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

Closed
wants to merge 59 commits into 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
Member

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
Member

@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
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe try compiling on a newer Node.js version and see if the issue persists. E.g., Node v20 or the latest LTS.

@headlessNode
Copy link
Member

@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

@stdlib-bot
Copy link
Contributor

stdlib-bot commented Nov 17, 2024

Coverage Report

Package Statements Branches Functions Lines
math/base/special/minmax $\color{green}195/195$
$\color{green}+100.00\%$
$\color{green}15/15$
$\color{green}+100.00\%$
$\color{green}2/2$
$\color{green}+100.00\%$
$\color{green}195/195$
$\color{green}+100.00\%$

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

@AbhijitRaut04
Copy link
Contributor Author

image
@headlessNode
@kgryte
@Planeshifter
Can you suggest me the updates in pr so that the above issue will get resolved ?

@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

/stdlib merge

@kgryte kgryte added Feature Issue or pull request for adding a new feature. C Issue involves or relates to C. labels Nov 18, 2024
@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

@AbhijitRaut04 This PR needs some updating. See https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/math/base/special/frexp for an example of how to cross from JS to C and back. The difference here will be that minmax is a binary function, not a unary function, but this should be straightforward to accommodate based on the structure of frexp.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Nov 18, 2024
@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

For the C API, we don't need stride and offset. Just provide an *out pointer and assume that the user has provided a two element array.

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/minmax
5 participants