-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Faster implementation of Float64(::BigInt), Float32(::BigInt) and Float16(::BigInt) #31502
Faster implementation of Float64(::BigInt), Float32(::BigInt) and Float16(::BigInt) #31502
Conversation
36a6e25
to
14edffa
Compare
78bb03e
to
8f5112e
Compare
In general looks pretty good: would be nice if we could make this more generic, as we will need to repeat it for Float32 and Float16. Need to also check that we cover good test cases as well. |
Currently, I am trying to refactor the code to remove unnecessary computations and the code 'prettier'. For generalization purposes, I am attempting to write code by using a variable Also work related to Float32 and Float16 should be done in this issue itself or a separate issue will be opened? |
Lets keep it all in the same pull request. To help make it generic, there are various utility functions that can be of use, e.g.: Lines 571 to 582 in 91151ab
Lines 846 to 873 in 91151ab
|
I will make sure to use them as much as possible. |
7c6de4b
to
a2eec20
Compare
a2eec20
to
eb99997
Compare
@simonbyrne I have implemented the conversion methods with |
But there is "test/bigint.jl". |
Yes, there is. 😅 |
@simonbyrne is this okay? |
In general, it looks pretty good. For each type
|
a84d770
to
0aa22e9
Compare
@simonbyrne I have updated the tests. Is there any way I can generalize other tests too? The idea I used is to test for conversion of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the existing definition of round-to-nearest conversion, and make it this one.
After this, I think it's good to go.
0aa22e9
to
faa9f96
Compare
@simonbyrne The PR has been updated accordingly. |
If this is okay, can we merge this? (before the 1.2 freeze 😄 ) @simonbyrne |
faa9f96
to
147ac92
Compare
Looks like there is a test failure on 32-bit Linux:
|
147ac92
to
d965683
Compare
@simonbyrne All the tests are now passing except the following:
|
@simonbyrne If this is okay, can this be merged? |
Closes #31293
Implements a faster version of
Float64(::BigInt)
,Float32(::BigInt)
andFloat16(::BigInt)
.Rounding Behaviour: RoundNearest