-
Notifications
You must be signed in to change notification settings - Fork 10
cleanups #92
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
cleanups #92
Conversation
simonbyrne
left a comment
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.
That's a much better idea, I don't know why I didn't do it this way originally.
|
Also, while I'm cleaning things up, Is the current approach of calling C functions for the basic conversions good? I see ~2x faster performance using the pure Julia UInt128 conversion than the ccalled UInt64 conversion (zen5 linux). If one of you has an aarc64 system (which has native Float128), can you run this benchmark? |
|
On my M2 Mac Yeah, so that looks faster... |
|
You can also clean up these bits as well: Lines 191 to 197 in f6ea87e
Lines 215 to 221 in f6ea87e
|
|
@simonbyrne I might have gone slightly overboard and moved a lot of the simple implementations into Julia with pretty decent perf gains (usually ~2x sometimes as big as 8x) |
|
so, should we merge this? |
|
I'll defer to @simonbyrne |
|
I'll say. Simon very nice grist for Float128. Floats where the edge [cases] have been honed to refract |
|
Let's bump the minor version (or make it 1.0) then it's good |
|
Lets go to 1.0! |
|
I do not agree with going to 1.0 until a few other issues are addressed. |
|
That's easy enough to fix. Would printing as |
|
|
Are we willing to rely on Base.Ryu for this? it already has the code and it works really well. |
|
Also, how would we feel about printing/showing these as |
|
"Also, how would we feel about printing/showing these as Inf and NaN rather than Inf128 and NaN128? That would match what we do for Inf32 and other 32 bit types." |
|
I pushed the changes, and then immediately tested with a non trivial case and it broke (Base.Ryu is fairly non generic and requires a bunch of internal functions to be defined to work). That said, I don't love the snprintf printing either since it doesn't provide a way of doing shortest printing to match Julia. I've moved it a bit closer to Julia printing now, but there are still a few quirks. For example |
|
so... merge? |
This removes a bunch of compats for unsupported Julia versions, and slightly simplifies the annoying hacks we're making to handle the ABI issues.