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

Add fast inverse square root algorithm #647

Merged
merged 5 commits into from
Sep 1, 2023

Conversation

yan-aint-nickname
Copy link
Contributor

Almost identical to C implementation but with golang specific unsafe package
https://en.wikipedia.org/wiki/Fast_inverse_square_root

siriak
siriak previously approved these changes Aug 24, 2023
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@siriak siriak enabled auto-merge (squash) August 24, 2023 16:07
Copy link
Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

Please make the changes to the comments as I suggested in the discussion.

@tjgurwara99
Copy link
Member

@yan-aint-nickname
Copy link
Contributor Author

@tjgurwara99 there is some problem with the algorithm what you send. 1/y division is very expensive operation that's why right-shift >> is used in floating point bit level hacking line. It's better to multiply by passed value. This algorithm was made to reduce divisions. And I return 1/y value not a y, because it's reverse square root. I'm going to clear comments @raklaptudirm , I just wanted to pay homage to the original algorithm. May I change what the f*** to what the hell? If you have any questions, please ask

@tjgurwara99
Copy link
Member

tjgurwara99 commented Aug 25, 2023

I was in a hurry so I didn't write a descriptive comment but I pointed out the other implementation not because I want you to remove your implementation but rather do it without unsafe. As you can see from that function it can be done!!

Furthermore, if you look closely, it is almost identical to your function so if you move it (and name it appropriately, eg InverseSqrt or something) and change the other function to be something like func Sqrt(n float32) float32 { return 1 / InverseSqrt(x) } you solved two problems:

  1. Simple is better than complex by removing the unsafe package, and
  2. Keeping similar logical blocks together in the same package.

@yan-aint-nickname
Copy link
Contributor Author

I was in a hurry so I didn't write a descriptive comment but I pointed out the other implementation not because I want you to remove your implementation but rather do it without unsafe. As you can see from that function it can be done!!

Furthermore, if you look closely, it is almost identical to your function so if you move it (and name it appropriately, eg InverseSqrt or something) and change the other function to be something like func Sqrt(n float32) float32 { return 1 / InverseSqrt(x) } you solved two problems:

  1. Simple is better than complex by removing the unsafe package, and
  2. Keeping similar logical blocks together in the same package.

Float32bits it's just an alias for *(*int32)(unsafe.Pointer(&y)) what I did wrong that I used int32 instead of uint32
https://github.com/golang/go/blob/master/src/math/unsafe.go#L12

So, I need to:

  1. Rename Q_rsqrt to FastInverseSqrt
  2. Move FastInverseSqrt to math/binary package
  3. Replace unsafe with simple Float32bits and Float32frombits
  4. Replace Sqrt in math/binary/sqrt.go with func Sqrt(n float32) float32 { return 1 / FastInverseSqrt(x) }
  5. Improve comments for better godoc generation

Am I right?

@tjgurwara99
Copy link
Member

tjgurwara99 commented Aug 25, 2023

So, I need to:

Rename Q_rsqrt to FastInverseSqrt
Move FastInverseSqrt to math/binary package
Replace unsafe with simple Float32bits and Float32frombits
Replace Sqrt in math/binary/sqrt.go with func Sqrt(n float32) float32 { return 1 / FastInverseSqrt(x) }
Improve comments for better godoc generation
Am I right?

Yes 😄

Float32bits it's just an alias for *(*int32)(unsafe.Pointer(&y)) what I did wrong that I used int32 instead of uint32
https://github.com/golang/go/blob/master/src/math/unsafe.go#L12

To be honest, even I didn't notice that you'd used int32 instead - one more reason why I avoid unsafe package (as much as I can)

auto-merge was automatically disabled August 27, 2023 14:26

Head branch was pushed to by a user without write access

tjgurwara99
tjgurwara99 previously approved these changes Aug 30, 2023
math/binary/fast_inverse_sqrt.go Outdated Show resolved Hide resolved
Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

LGMT

@tjgurwara99 tjgurwara99 enabled auto-merge (squash) September 1, 2023 07:25
@tjgurwara99 tjgurwara99 merged commit f7ca03b into TheAlgorithms:master Sep 1, 2023
3 checks passed
@yan-aint-nickname yan-aint-nickname deleted the feature/q_sqrt branch September 1, 2023 19:31
SuperDev0514 added a commit to SuperDev0514/Go that referenced this pull request May 16, 2024
* Add fast inverse square root algorithm

* PR improvements see comments in TheAlgorithms/Go#647

* Update math/binary/fast_inverse_sqrt.go file docstring

Co-authored-by: Taj <[email protected]>

* Update math/binary/fast_inverse_sqrt.go function documentation

Co-authored-by: Taj <[email protected]>

* Fix function documentation grammar

Co-authored-by: Taj <[email protected]>

---------

Co-authored-by: Taj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants