Added function to show Rational at a decimal precision#2945
Added function to show Rational at a decimal precision#2945aortega0703 wants to merge 3 commits intoagda:masterfrom
Conversation
gallais
left a comment
There was a problem hiding this comment.
Same comments for the Normalised version too.
|
|
||
| -- The integer part of q, | ||
| -- followed by the first n digits of its decimal representation | ||
| atPrecision : (n : ℕ) → ℚᵘ → Vec ℤ (suc n) |
There was a problem hiding this comment.
We surely want this to return ℤ × Vec ℕ n
There was a problem hiding this comment.
Or even, ℤ × Vec Decimal n, ie. ℤ × Vec (Digit 10) n?
Is it possible (better!?) to re-use any of the functionality in Data.Digit, such as toDigits, rather than computing from scratch? Actually, maybe such an approach would only work for the normalised representation, but I confess I'm no longer clear why unnormalised rationals might need a separate show function, rather than simply 'normalise-then-show'?
There was a problem hiding this comment.
We have had bad experiences with Digit being slow hence why I refrained from
suggesting it. Cf. #678
There was a problem hiding this comment.
I also though about multiplying the fractional part by 10^n, which should give me a ℕ with the first n decimal digits after truncating (and maybe calling toDigits on that?). But the "multiplying by 10^n" part would make the intermediate ℚ have a big numerator so I don't know if that's desirable. Should I do this or use an absolute value and fix the return type?
src/Data/Rational/Show.agda
Outdated
| showAtPrecision n q with atPrecision n q | ||
| ... | int ∷ dec = ℤ.show int ++ "." ++ concat (toList (map ℤ.show dec)) |
There was a problem hiding this comment.
Since Vec also supports each of map and concat operations, I'm not sure why you need the extra toList here; moreover, since you already observe that dec here consists of digits (which would appear as one-character Strings), it looks as though there would opportunities to streamline the RHS of this definition? Eg. using Data.String.fromVec?
|
I'm definitely perturbed by
Such concerns are, strictly speaking, out of scope for this PR, but I think we should bear them in mind when implementing (internal) computations intended to be used 'in anger', such as show functions...? UPDATED: I'm going to lift this out as a separate issue #2946 and not let it get in the way of this PR. |
jamesmckinna
left a comment
There was a problem hiding this comment.
Thanks very much for this first PR!
Some suggestions and other comments, but I think better to merge sooner rather than later.
| showAtPrecision : ℕ → ℚᵘ → String | ||
| showAtPrecision n q = ℚ.showAtPrecision n (fromℚᵘ q) |
There was a problem hiding this comment.
Thanks for doing this.
I realise it violates the 'usual' heuristics of the dependency graph (Data.Rational delegates to/is a normalised wrapper around Data.Rational.Unnormalised), but here I think it does make sense/is the right choice, even if only from a DRY point of view.
| showAtPrecision n q with atPrecision n q | ||
| ... | (int , dec) = ℤ.show int ++ "." ++ (fromVec (map toDigitChar dec)) |
There was a problem hiding this comment.
Two things:
- you can streamline the irrefutable pattern match on the
witheither vialet, or perhaps preferable, the newusingsyntax - there's an annoying corner case: if
decis[], do you leave the generated string as""(so, eg"3."), or catch it and render as.0?
For the first:
| showAtPrecision n q with atPrecision n q | |
| ... | (int , dec) = ℤ.show int ++ "." ++ (fromVec (map toDigitChar dec)) | |
| showAtPrecision n q using int , dec ← atPrecision n q | |
| = ℤ.show int ++ "." ++ (fromVec (map toDigitChar dec)) |
For the second, I guess I'm not so prescriptive ;-)
| decimalPart (suc n) q | ||
| = ∣ truncate q ∣ ∷ decimalPart n ((+ 10 / 1) * (fracPart q)) |
There was a problem hiding this comment.
This seems fine on a first pass, ... and premature optimisation is the root of all evil ;-)
#1481 I added functions to show the decimal representation of a
Data.Rational(orData.Rational.Unnormalised) withndecimal digits.