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

Added Sqrt and SqrtRound functionality #130

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ldcicconi
Copy link

Based this on #111 , but corrected an algorithmic bug.

Not sure why those other changes are showing up.

@huahuayu
Copy link

huahuayu commented Dec 1, 2020

I need decimal to be sqrt(), it's current missed, please merge the function

@mwoss mwoss mentioned this pull request Mar 25, 2021
@mwoss
Copy link
Member

mwoss commented Mar 26, 2021

The implementation overall looks good. If you can @ldcicconi, please rebase it so I can merge it to master and add unit tests for this implementation. If you don't have time I can do it and write unit tests for this implementation.

@ldcicconi
Copy link
Author

@mwoss on it.

@ldcicconi
Copy link
Author

@mwoss I added some unit tests and made a few other changes. Let me know what you think.

note: the second test case in the vals map currently fails. I must be missing something for numbers near 0.

@mwoss
Copy link
Member

mwoss commented Mar 30, 2021

Hmm, I'm not sure why this algorithm is failing on numbers in the range (0, 1). What method you have used for calculating sqrt? Can you point me to the paper or any article?

@qiquanlu
Copy link

Can we get this PR merged? The code looks good, only issue is a couple digit off, and it's way more than enough for most use cases. #222

decimal_test.go:2913: Square root of 0.002342 should be 0.0483942145302514, not 0.0483942145302509 (error: %!s())

thanks

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.

None yet

4 participants