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.

Sign up for GitHub

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

Jump to bottom

feat: add math/base/special/atan2f #3338

Draft
wants to merge 24 commits into
base: develop
Choose a base branch
from
Draft

feat: add math/base/special/atan2f #3338

wants to merge 24 commits into from

Conversation

Neerajpathak07
Copy link
Contributor

@Neerajpathak07 Neerajpathak07 commented Dec 5, 2024 •
edited
Loading

Resolves #649

Description

What is the purpose of this pull request?

This pull request:

  • proposes adding math/base/special/atan2f , which would be a single precision implementation for math/base/special/atan2.

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 Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Dec 5, 2024
@Neerajpathak07 Neerajpathak07 marked this pull request as draft December 5, 2024 14:29
@stdlib-bot stdlib-bot removed the Needs Review A pull request which needs code review. label Dec 5, 2024
@Neerajpathak07 Neerajpathak07 changed the title [RFC]: add math/base/special/atan2f feat: add math/base/special/atan2f Dec 5, 2024
@aayush0325
Copy link
Member

i see that this PR is failing a lot of tests. you should update your manifest.json to include all the necessary single-precsion dependencies to use them in src/ and lib/ currently you are including double precision dependencies but using the single-precision ones which is why this is causing an error.

@aayush0325
Copy link
Member 

"dependencies": [
        "@stdlib/math/base/napi/binary",
        "@stdlib/math/base/assert/is-infinitef",
        "@stdlib/math/base/special/copysignf",
        "@stdlib/number/float32/base/signbit",
        "@stdlib/math/base/assert/is-nanf",
        "@stdlib/math/base/special/atanf",
        "@stdlib/constants/float32/pinf",
        "@stdlib/constants/float32/pi"
      ]

this is how you should fill the dependencies in manifest.json

@Neerajpathak07
Copy link
Contributor Author

@aayush0325 thanks for the assist.

@aayush0325
Copy link
Member

use number/float32/base/signbit instead of number/float32/base/signbitf as the latter doesn't exist, checkout the package here

@Planeshifter Planeshifter added the status: Blocked Issue or pull request which is current blocked. label Dec 10, 2024
@Planeshifter
Copy link
Member

Planeshifter commented Dec 10, 2024 •
edited
Loading

@Neerajpathak07 The issue here is that @stdlib/number/float32/base/signbit doesn't yet have a C implementation. This PR will have to wait until we have that C implementation.

@Neerajpathak07 Neerajpathak07 mentioned this pull request Dec 23, 2024
Merged
1 task
@Neerajpathak07
Copy link
Contributor Author

@Planeshifter The PR related to float32/signbit has been merged could you unblock this PR

Neerajpathak07
Neerajpathak07 commented Jan 15, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/atan2f/lib/main.js
*
* ## Notice
*
* The original code, copyright and license are from [Go]{@link https://golang.org/src/math/atan2.go}. The implementation follows the original, but has been modified for JavaScript.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kgryte
Apparently there was no golang source link for atan2f which is why I have kept it atan2 for now.

@Planeshifter Planeshifter removed the status: Blocked Issue or pull request which is current blocked. label Jan 15, 2025
aayush0325
aayush0325 reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@aayush0325 aayush0325 left a comment

Choose a reason for hiding this comment

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

basically you need to generate the fixtures using the julia script and not copy them over from the double precision implementation

lib/node_modules/@stdlib/math/base/special/atan2f/README.md Outdated
Computes the angle in the plane (in radians) between the positive x-axis and the ray from `(0,0)` to the point `(x,y)` for single-precision floating-point number.

```c
float out = stdlib_base_atan2f( 2.0,f 2.0f );
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
float out = stdlib_base_atan2f( 2.0,f 2.0f );
float out = stdlib_base_atan2f( 2.0f, 2.0f );

lib/node_modules/@stdlib/math/base/special/atan2f/src/main.c Outdated
#include "stdlib/constants/float32/pinf.h"
#include "stdlib/constants/float32/ninf.h"
#include "stdlib/constants/float32/pi.h"
#include <stdint.h>
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
#include <stdint.h>

i don't think this header is being used

lib/node_modules/@stdlib/math/base/special/atan2f/test/fixtures/julia/negative_negative.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

ahh, these fixtures are probably the cause of failing tests here, you've copied these over from the double precision implementation as I compared the both and there is no diff. You are bound to face errors if you test your single precision implementation against a double precision dataset. CD into the dir and generate these fixtures by running runner.jl you may need to setup Julia and install JSON package for that first

Copy link
Member

Choose a reason for hiding this comment

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

holding out on further review before this is resolved, this should be it i think

@aayush0325
Copy link
Member

The reason for the failing benchmarks is probably because you copied over the Makefile wrongly, use the one in math/base/special/atan2/benchmark/c/native/

@aayush0325
Copy link
Member

Makefile that you should be using. This adds the include dirs so that you don't run into the error that we're currently facing here

kgryte
kgryte reviewed Jan 20, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/atan2f/benchmark/c/native/Makefile Outdated
@@ -1,4 +1,4 @@
#/

Copy link
Member

Choose a reason for hiding this comment

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

This change is not correct. The comment should begin with #/.

@Neerajpathak07 Neerajpathak07 mentioned this pull request Jan 25, 2025
Draft
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aayush0325 aayush0325 aayush0325 left review comments

@kgryte kgryte kgryte left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Math Issue or pull request specific to math functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: Add C implementations to base special math functions (tracking issue)
5 participants
@Neerajpathak07 @aayush0325 @Planeshifter @kgryte @stdlib-bot