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

math/big: Rat: add FloatPrec() (int, bool) #50489

Open
mitar opened this issue Jan 7, 2022 · 24 comments
Open

math/big: Rat: add FloatPrec() (int, bool) #50489

mitar opened this issue Jan 7, 2022 · 24 comments

Comments

@mitar
Copy link
Contributor

mitar commented Jan 7, 2022

Update, Mar 2 2022: Current API is at #50489 (comment). -rsc


Currently if I parse string like 123.34 with SetString() and want to print it back to full precision, one has to manually pick the right precision for FloatString() function to get that. I propose that instead FloatString() should accept negative prec parameter. The full semantics of prec parameter would then be:

  • For positive prec, it formats with prec digits after the radix point, padding with 0 on the right if necessary. The last digit is rounded to nearest, with halves rounded away from zero, if rounding is necessary.
  • When prec is 0, there are no digits after the radix point.
  • If prec is less than 0, then:
    • If there is a finite number of digits after the radix point to precisely represent the number, all these digits are appended after the radix point.
    • If the number of digits is infinite, then abs(prec) number of digits is used after the radix point, rounded the last digit to nearest, with halves rounded away from zero.

So FloatString(3) for the number above returns 123.340, but FloatString(-3) return 123.34.

big.NewRat(1, 3).FloatString(3) returns 0.333 and big.NewRat(1, 3).FloatString(-3) would return 0.333.

@mitar mitar added the Proposal label Jan 7, 2022
@gopherbot gopherbot added this to the Proposal milestone Jan 7, 2022
@robpike
Copy link
Contributor

robpike commented Jan 7, 2022

I wrote a very long and complicated answer to this, but perhaps it's better just to state that rather than change FloatString in such a peculiar and possibly incompatible way, a better answer is to add a function that tells you how many bits of precision you'd need. The problem with that is that there is no simple answer in general, as you may have a repeating decimal, but the function could tell you that:

func (r *Rat) FloatSize() (digitsLeftOfDecimal, digitsRightOfDecimal int, repeats bool)

That solves the problem but I am far from convinced the problem is significant enough to require such a messy fix, either mine or yours. If you want to be clever, you can compute (10 gcd denominator) and get some of the answer yourself, since infinite-length representation only happens when the factors of the denominator are not all factors of the printing base.

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2022

That is a good point and I like the idea of FloatSize.

I am far from convinced the problem is significant enough to require such a messy fix

With messy fix you mean the negative precision proposal or FloatSize?

@robpike
Copy link
Contributor

robpike commented Jan 7, 2022

Both. As I said, "mine or yours".

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2022

I see.

The problem I have is that when marshaling and unmarshaling JSON with large decimal numbers as strings it is useful to have a way to parse them and then marshal them back into the same precision as they came in. Because they are parsed from a fixed length strings I know that I can format them back to one as well (so that there will be no repeats or infinite digits). The issue is only that there is no easy way to do that back conversion. On the other hand I do not want to lose precision if it is not necessary.

So having FloatSize would address that for me.

@ianlancetaylor ianlancetaylor changed the title proposal: math/big.Rat: Negative precision for full precision string representation proposal: math/big: Rat: negative precision for full precision string representation Jan 7, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 7, 2022
@mitar
Copy link
Contributor Author

mitar commented Jan 13, 2022

I made an implementation with signature func RatPrecision(rat *big.Rat) (int, int), where the first int is the number of non-repeating digits after decimal dot, and the second the number of repeating (cycling) digits which follow. Seems to work pretty well, maybe it could be included into stdlib.

@rsc
Copy link
Contributor

rsc commented Feb 2, 2022

This seems like an awkward use of big.Rat. Why use big.Rat in this case at all? Why not use json.Number as the round-trippable representation?

@mitar
Copy link
Contributor Author

mitar commented Feb 2, 2022

Oh, sorry if that was unclear from the proposal. The roundtrip is the sanity check motivation, if I have a data structure and I read it from the JSON into numbers with big.Rat as fields, I would at least like to be able to generate back the original JSON with some ease. And currently that is hard. Of course, the main use is that after parsing JSON number info a struct with big.Rat field, one would take that and do further processing on the struct. Potentially later on try to save it to JSON, but at that point values might not be representable without losing some precision. That is fine, but I would at least like to be able to save it to JSON without losing precision when that is not necessary (e.g., when there is a finite number of digits). But currently it is not possible using stdlib to render the big.Rat to a number knowing that you are not losing precision (when that is possible).

@rsc
Copy link
Contributor

rsc commented Feb 9, 2022

Do we care about the repeat size or the digits on the left?
The repeat size of N/D in particular can be D and therefore requires a big.Int to represent.
And the digits on the left is irrelevant to using FloatPrec.

The finite digits on the right is guaranteed to be 10 log D, which will fit in an int whenever D fits in memory.
So perhaps we should have

func (r *Rat) FloatPrec() (digits int, ok bool)

that returns 0, false for rats with non-finite float representations?

@mitar
Copy link
Contributor Author

mitar commented Feb 9, 2022

Yes, I agree that digits on the left are not important. The implementation I made here has the following signature:

func (r *Rat) FloatPrec() (digits int, repeating int)

So repeating == 0 when your ok == true.

But maybe my implementation is not as elegant as it could be.

But I would be OK with func (r *Rat) FloatPrec() (digits int, ok bool) as well.

In practice, using the signature from my implementation, I generally do l+j number of digits, so all non-repeating digits followed by one cycle of repeating digits.

@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

@mitar the problem is that there can be >2^64 repeating digits.

@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@mitar
Copy link
Contributor Author

mitar commented Feb 16, 2022

the problem is that there can be >2^64 repeating digits.

Sure, the caller of this function would then decide what limits to apply before passing digits further. Or are you saying that int return value type is not enough? I think yes, we could flag somehow if that would overflow or something.

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

Yes, the problem is the result does not fit in int, or even int64.
Is the repeat count really necessary? I would argue not, in which case we should do just

func (r *Rat) FloatPrec() (digits int, ok bool)

@mitar
Copy link
Contributor Author

mitar commented Feb 23, 2022

I am fine with that.

@rsc rsc changed the title proposal: math/big: Rat: negative precision for full precision string representation proposal: math/big: Rat: add FloatPrec() (int, bool) Mar 2, 2022
@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

For clarity:

 (1/10).FloatPrec() = 1, true
 (10/100).FloatPrec() = 1, true
 (3/100).FloatPrec() = 2, true
 (1/3).FloatPrec() = 0, false
 (10).FloatPrec() = 0, true

@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

Does anyone object to FloatPrec as described in #50489 (comment)?

@mitar
Copy link
Contributor Author

mitar commented Mar 2, 2022

Few more examples:

(1/3).FloatPrec() = 0, false
(1/6).FloatPrec() = 1, false
(1/7).FloatPrec() = 0, false
(1/9).FloatPrec() = 0, false
(1/28).FloatPrec() = 2, false
(1/67).FloatPrec() = 0, false
(1/81).FloatPrec() = 0, false
(1/96).FloatPrec() = 5, false
(8/13).FloatPrec() = 0, false
(2/14).FloatPrec() = 0, false
(3/30).FloatPrec() = 1, true
(2/3).FloatPrec() = 0, false
(9/11).FloatPrec() = 0, false
(7/12).FloatPrec() = 2, false
(22/7).FloatPrec() = 0, false

@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

I disagree about the above examples. If the bool is false, the count is always zero.
The bool distinguishes only 0, false (some infinite eventually repeating decimal)
from 0, true (an integer with no digits after the decimal point).

The number of digits before the repetition begins may not fit in an int64 (I'm not sure)
but also seems like not a useful number to have. I also don't know how to compute it efficiently.

If there is some trivial computation of the non-repeated prefix and it is guaranteed to fit in int64 and it's useful, then maybe we could think about adding it. But none of those three seem to be true.

@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

With that clarification, does anyone object to adding FloatPrec as in #50489 (comment)?

@robpike
Copy link
Contributor

robpike commented Mar 9, 2022

I am still not sure this problem is worth solving at all due to its rarity in practice.

@rsc
Copy link
Contributor

rsc commented Mar 16, 2022

The original message said this was the use case:

Currently if I parse string like 123.34 with SetString() and want to print it back to full precision

The roundtrip is the sanity check motivation, if I have a data structure and I read it from the JSON into numbers with big.Rat as fields, I would at least like to be able to generate back the original JSON with some ease.

I am sympathetic to that and can see it arising in a variety of programs.

In fact, it is a bit annoying to me in Ivy that 0.1 + 0.1 = 1/5 instead of 0.2.
This would give Ivy a way to print shorter decimals when that's still precise.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Mar 23, 2022
@rsc
Copy link
Contributor

rsc commented Mar 23, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 30, 2022
@rsc
Copy link
Contributor

rsc commented Mar 30, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: math/big: Rat: add FloatPrec() (int, bool) math/big: Rat: add FloatPrec() (int, bool) Mar 30, 2022
@rsc rsc modified the milestones: Proposal, Backlog Mar 30, 2022
@gopherbot
Copy link

Change https://go.dev/cl/521235 mentions this issue: math/big: Rat: add FloatPrec() (int, bool)

@panjf2000 panjf2000 self-assigned this Aug 20, 2023
@panjf2000 panjf2000 modified the milestones: Backlog, Go1.22 Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

5 participants