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

Improve IEEE float decimal processing #45

Closed
wants to merge 11 commits into from

Conversation

winterland1989
Copy link
Contributor

@winterland1989 winterland1989 commented Feb 24, 2017

@winterland1989 winterland1989 changed the title Improve ieee float decimal processing Improve IEEE float decimal processing Feb 24, 2017
@cartazio
Copy link

I like the goal of this ...

  1. what does round do? Looks like it'd be a noop, because should have some additional info about number of bits of precision in significand and mantissa. And that's ignoring that there's like 4+ ieee rounding modes.

On the rendering side, have you seen the algorithmic that ranjit et al presented at popl 2016?

@winterland1989
Copy link
Contributor Author

winterland1989 commented Feb 24, 2017

what does round do? Looks like it'd be a noop, because should have some additional info about number of bits of precision in significand and mantissa. And that's ignoring that there's like 4+ ieee rounding modes.

Good question, I should use round half to even to be more explict about rouding mode. This is also the current rounding mode round use. I expected roundFloat 1.2 == 1.0 so it's not an noop of course.

Actually current round 's primop is implement as a IEEE float to IEEE float C function.

On the rendering side, have you seen the algorithmic that ranjit et al presented at popl 2016?

Yes, but it turned out they did a false benchmark. We should use Grisu3.

@ghost
Copy link

ghost commented Feb 26, 2017

This article by BOS does a good job at explaining the motivation behind this PR.

@cartazio
Copy link

i'm still confused about what round is supposed to do...

@winterland1989
Copy link
Contributor Author

winterland1989 commented Feb 26, 2017

@siddhanathan Thanks for bringing that up ; )

@cartazio I have updated the comment of roundFloat function in the proposal, is it clear to you now?

@cartazio
Copy link

Oh. Rounding to nearest integer.

Zooming out: what's the problem we are trying to solve in base libraries here?

@winterland1989
Copy link
Contributor Author

winterland1989 commented Feb 27, 2017

Currently in base we have round :: (Integral b, RealFrac a) => a -> b which force us return result as an Integral, but this makes all IEEE Float algorithms using round became inefficient because we have to convert these Integral back and forth.

We can't really change round, since Double/Float are not the only instances in RealFrac, but we can add a special rounding method for RealFloat since it's intended for dealing with IEEE float, we can even expose rounding mode to user if there's a need.

I'm also considering to add ceilingFloat, floorFloat and truncateFloat to match functions in C math.h. In short, i think IEEE float worth separated processing considerations rather than got mixed with Integral a => Ratio a or HasResolution a => Fixed a, since their representation and usage are fundamentally different.

@cartazio
Copy link

One possible near term thing:: maybe this would make sense to add to the ieee754 package. It looks like @patperry (i think that's his handle or @patrickperry? ) has resumed maintaining that and it has a nice type class just for ieee floats and friends

@alexanderkjeldaas
Copy link

decodeFloatDecimal returns [Word8], but isn't there a bounded length to this list? As long as this bound is reasonable and the list representation has something like a 30x(?)-ish overhead, the representation seems suboptimal. The output would never be lazy, would it? How about using a ByteString, Array or Data.Vector?

@winterland1989
Copy link
Contributor Author

@alexanderkjeldaas Sadly there is no proper compact representation in base now.

@winterland1989
Copy link
Contributor Author

I also would like to hear @patperry 's thought on this proposal, it's better to have complete IEEE float processing tools in base IMHO.

I don't know if this proposal is too urgent for 8.2 release, if that's the case i will modify the plan.

@cartazio
Copy link

cartazio commented Feb 28, 2017 via email

@patperry
Copy link

patperry commented Feb 28, 2017

I don't have a strong opinion either way regarding whether this should be part of base.

In the near-term, I'm happy to add this to ieee754 if you send me a pull request.

@patperry
Copy link

(I'm also happy if you want to fork/take over the ieee754 package yourself--I don't really have time to actively maintain it. You may want to make some major changes to the typeclass, which I wouldn't object to as long as you retain the functionality. Just make sure you give the package authors and contributors appropriate credit.)

@winterland1989
Copy link
Contributor Author

@patperry I'm happy to take over the ieee754 package ; ), please add me to the maintainers.

I have updated the proposal implementation plan, let me start improving IEEE float situation by improving ieee754 first.

@patperry
Copy link

patperry commented Mar 3, 2017

@winterland1989 sounds like a great plan. Please go ahead and fork https://github.com/patperry/hs-ieee754 , then add the functionality. I'll look over your changes, and give you feedback (likely pretty minimal). If you can address the feedback, then I'll add you as a maintainer.

I'm really glad that you're taking the initiative to improve the float situation, and I look forward to seeing your changes.

@gridaphobe
Copy link
Contributor

On the rendering side, have you seen the algorithmic that ranjit et al presented at popl 2016?

Yes, but it turned out they did a false benchmark. We should use Grisu3.

I just asked my labmate about this (he's the author), and he says that since the initial publication he's been able to improve performance to within 10% of Grisu3. Furthermore, someone sent him a patch that supposedly beats Grisu3 by 5%. He's working on incorporating the patch, so it might be worth investigating further. Especially given that his approach doesn't rely on a backup rendering algorithm, sounds like a nice code simplification.

@minad
Copy link

minad commented Mar 7, 2017

@winterland1989 @patperry Most of the relevant floating point operations are already here it seems https://github.com/ekmett/numeric-extras/blob/master/Numeric/Extras.hs

@winterland1989
Copy link
Contributor Author

Yep, one of this proposal's goals is to bring operations which standard c lib already provided. But I'm not sure if we can directly import them from math.h since ghc is supposed to support many architectures , maybe someone worked for ghc cross architectures can show some light here?

@alexanderkjeldaas
Copy link

Maybe this is off-topic, but if everybody is in agreement that APIs using String is bad, but it has to be used because nothing else is in base, then shouldn't there be some future-proof way out of this situation?

For example a typeclass representing alternative future String implementations, or naming the function with some suffix like getFoobarAsString.

I guess this isn't as much a comment on this particular proposal as a question of how base should prepare for a better String at some point in the future.

@cartazio
Copy link

cartazio commented Mar 21, 2017 via email

@nomeata
Copy link
Contributor

nomeata commented May 7, 2017

I get the impression that this is not really a GHC proposal, but rather a base library proposal, in which case it should be submitted to the Core Library Committee as described in https://wiki.haskell.org/Library_submissions.

The proposal says that this code will be developed in a separate library first. This is a good approach! Adoption of such a library will be a strong argument in favor of accepting this proposal.

@nomeata nomeata added the Out-of-scope The requested change is not a matter for the GHC Steering Committee to decide label May 25, 2017
@nomeata
Copy link
Contributor

nomeata commented May 25, 2017

Since nobody disagrees with my previous post, I am closing this as Out-Of-Scope. But please do pursue the problem, probably by implementing the library, getting users, and turning it into a library submission.

@nomeata nomeata closed this May 25, 2017
@winterland1989
Copy link
Contributor Author

Yes please, I'll come back to this when i get time ; )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Out-of-scope The requested change is not a matter for the GHC Steering Committee to decide
Development

Successfully merging this pull request may close these issues.

None yet

8 participants