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

refactor: update math/base/special/atanh according to the FreeBSD implementation #3128

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

refactor: update math/base/special/atanh according to the FreeBSD implementation #3128

wants to merge 13 commits into from
+12 −12

Conversation

RiyaChy072
Copy link

@RiyaChy072 RiyaChy072 commented Nov 17, 2024 •
edited
Loading

Resolves #2089 .

Description

What is the purpose of this pull request?

This pull request:

  • converted the floating-point value to a word and then performed integer calculations, as given in the original FreeBSD implementation.

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
Copy link
Contributor

Hello! Thank you for your contribution to stdlib.

We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:

  1. Please read our contributing guidelines.

  2. Update your pull request description to include this checked box:

    - [x] Read, understood, and followed the [contributing guidelines](https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md)

This acknowledgment confirms that you've read the guidelines, which include:

  • The developer's certificate of origin
  • Your agreement to license your contributions under the project's terms

We can't review or accept contributions without this acknowledgment.

Thank you for your understanding and cooperation. We look forward to reviewing your contribution!

@stdlib-bot
Copy link
Contributor

stdlib-bot commented Nov 17, 2024 •
edited
Loading

Coverage Report

Package Statements Branches Functions Lines
math/base/special/atanh $\color{green}248/248$
$\color{green}+100.00\%$ 		$\color{green}24/24$
$\color{green}+100.00\%$ 		$\color{green}2/2$
$\color{green}+100.00\%$ 		$\color{green}248/248$
$\color{green}+100.00\%$

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

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

@RiyaChy072 Thanks for working on this. But, declaring the numbers as constants, and then using them - is not what we actually want to do here. If you check the implementation of math/base/special/atanh , we haven't used the functions that are being used in the reference implementation. For instance, the reference implementation uses functions such as EXTRACT_WORDS and SET_HIGH_WORD, which are used to manipulate the "words" (a part of the bit pattern of a floating-point number, not a literal "word"). We need to use similar stdlib equivalent functions.

@RiyaChy072
Copy link
Author

RiyaChy072 commented Nov 17, 2024 •
edited
Loading

@gunjjoshi Understood. I will try to rectify that. Thanks

@RiyaChy072
Copy link
Author

@gunjjoshi Can you kindly check if it's done or not now?

@Planeshifter Planeshifter changed the title feat: update math/base/special/atanh according to the FreeBSD implementation feat: update math/base/special/atanh according to the FreeBSD implementation Nov 17, 2024
@kgryte kgryte changed the title feat: update math/base/special/atanh according to the FreeBSD implementation refactor: update math/base/special/atanh according to the FreeBSD implementation Nov 17, 2024
@kgryte kgryte added the Needs Review A pull request which needs code review. label Nov 18, 2024
@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

/stdlib merge

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Dec 11, 2024
@kgryte
Copy link
Member

kgryte commented Dec 11, 2024

Thanks for working on this, @RiyaChy072. However, it looks like this PR still needs some work. Notably, you need to use various stdlib utilities for extracting and setting words, etc, as Gunj mentioned above. My recommendation is to spend some time analyzing other C implementations in math/base/special and then replicating those ideas here. As is, this PR won't be able to move forward.

@RiyaChy072
Copy link
Author

@gunjjoshi Can you check if it's done ?

gunjjoshi
gunjjoshi reviewed Dec 27, 2024
View reviewed changes
lib/node_modules/@stdlib/math/base/special/atanh/scripts/fixtures/julia/data.json
Copy link
Member

Choose a reason for hiding this comment

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

@RiyaChy072 I don't think we need to change these fixtures for this. Was there any specific reason to update them?

Copy link
Author

Choose a reason for hiding this comment

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

@gunjjoshi I thought that needed to be done, didn't know we don't need that

gunjjoshi
gunjjoshi reviewed Dec 27, 2024
View reviewed changes
lib/node_modules/@stdlib/math/base/special/atanh/src/main.c
Comment on lines +85 to +93

if( ax > 1 || stdlib_base_is_nan( x ) ) { // |x|>1 or x is NaN
return 0.0 / 0.0 ; //NaN
}
if ( x == -1.0 ) {
return STDLIB_CONSTANT_FLOAT64_NINF;

if( ax == 1 ) { // |x| == 1
return (sgn > 0) ? STDLIB_CONSTANT_FLOAT64_NINF : STDLIB_CONSTANT_FLOAT64_PINF;
}

Copy link
Member

Choose a reason for hiding this comment

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

@RiyaChy072 This isn't what we need to do here. As mentioned previously, we need to use various stdlib utilities here, similar to how they are being used in the FreeBSD reference implementation.

Copy link
Author

Choose a reason for hiding this comment

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

@gunjjoshi Can i use HIGH_WORD_ABS_MASK here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gunjjoshi gunjjoshi gunjjoshi 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. Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: update math/base/special/atanh according to the FreeBSD implementation
4 participants
@RiyaChy072 @stdlib-bot @gunjjoshi @kgryte