Skip to content

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

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
Closed
wants to merge 31 commits into from
Closed

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

wants to merge 31 commits into from

Conversation

Neerajpathak07
Copy link
Member

@Neerajpathak07 Neerajpathak07 commented Dec 5, 2024 •
edited
Loading

Progresses #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
Member 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 currently 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
Member Author

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

@Planeshifter Planeshifter removed the status: Blocked Issue or pull request which is currently 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

@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

@Neerajpathak07 Neerajpathak07 mentioned this pull request Jan 25, 2025
Closed
1 task
Neerajpathak07
Neerajpathak07 commented Feb 11, 2025
View reviewed changes
lib/node_modules/@stdlib/math/base/special/atan2f/test/fixtures/julia/runner.jl
Comment on lines +41 to +43
z = Array{Float32}( undef, length(x) );
for i in eachindex(x)
z[ i ] = atan.( y[i], x[i] );
Copy link
Member Author

Choose a reason for hiding this comment

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

Julia doesn't have support for computing atan2f and atan2 so have kept it atan cuz that's the only one we have. But have generated it for float32

@Neerajpathak07
Copy link
Member Author

@aayush0325 Well the error still persists even after generating fixtures for julia.

@Neerajpathak07
chore: update benchmark
93fbc17
@Neerajpathak07
chore: update benchmark
f6611fd 
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
---
@Neerajpathak07
Merge branch 'add-atan2f' of https://github.com/Neerajpathak07/stdlib …
c13285b 
…into add-atan2f
@Neerajpathak07
chore: update fixtures
a0fc498
@Neerajpathak07
chore: minor changes
82f1843
@Neerajpathak07
Copy link
Member Author

@Planeshifter Only two test cases are failing in test.native.js whilst every tests and running-clean in test.js. Is there something I am missing out??

@Planeshifter Planeshifter added the Potential Duplicate There might be another pull request resolving the same issue. label Mar 15, 2025
@stdlib-bot stdlib-bot removed the Potential Duplicate There might be another pull request resolving the same issue. label Apr 19, 2025
@anandkaranubc
Copy link
Contributor

Completed via #7081

@anandkaranubc anandkaranubc added the autoclose: Already Resolved Pull request which should be auto-closed due proposed changes duplicating already included changes. label Jun 1, 2025
@stdlib-bot
Copy link
Contributor

Thank you for working on this pull request. However, we cannot accept your contribution as the issue this pull request seeks to resolve has already been addressed in a different pull request or commit.

Thank you again for your interest in stdlib, and we look forward to reviewing your future contributions.

@stdlib-bot stdlib-bot closed this Jun 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte kgryte left review comments

@aayush0325 aayush0325 aayush0325 left review comments

Assignees
No one assigned
Labels
autoclose: Already Resolved Pull request which should be auto-closed due proposed changes duplicating already included changes. 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.
6 participants
@Neerajpathak07 @aayush0325 @Planeshifter @anandkaranubc @stdlib-bot @kgryte