-
Notifications
You must be signed in to change notification settings - Fork 6
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 show
and string
#183
Conversation
It now defines `string` methods for the above types, instead of directly defining `show`. The string methods take as keyword arguments the possible flags to the printing functions in Flint. Arf now uses the function `arf_get_string` instead of converting to BigFloat. This allows handling numbers with exponents outside the range for BigFloat. The `string_nice` methods have been removed, in favour of the above string methods. In fact the string_nice never worked for Mag and Arf.
What previously was printed as (536870912 * 2^-29) is now printed as, the significantly more helpful, 1.00000000.
This matches the setting for Float64.
Oh, and maybe this is also a good time to publish |
src/show.jl
Outdated
function Base.string( | ||
x::MagOrRef; | ||
digits::Integer = digits_prec(30), | ||
remove_trailing_zeros::Bool = true, | ||
) | ||
cstr = ccall(@libflint(arf_get_str), Ptr{UInt8}, (Ref{arf_struct}, Int), Arf(x), digits) | ||
str = unsafe_string(cstr) | ||
ccall(@libflint(flint_free), Nothing, (Ptr{UInt8},), cstr) | ||
|
||
return remove_trailing_zeros ? _remove_trailing_zeros(str) : str | ||
end |
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 could just replace this with
function Base.string( | |
x::MagOrRef; | |
digits::Integer = digits_prec(30), | |
remove_trailing_zeros::Bool = true, | |
) | |
cstr = ccall(@libflint(arf_get_str), Ptr{UInt8}, (Ref{arf_struct}, Int), Arf(x), digits) | |
str = unsafe_string(cstr) | |
ccall(@libflint(flint_free), Nothing, (Ptr{UInt8},), cstr) | |
return remove_trailing_zeros ? _remove_trailing_zeros(str) : str | |
end | |
function Base.string( | |
x::MagOrRef; | |
digits::Integer = digits_prec(30), | |
remove_trailing_zeros::Bool = true, | |
) | |
return string(Arf(x); digits, remove_trailing_zeros) | |
end |
I actually didn't know that one can pass kwargs without explicitly writing them! when was it introduced?!
src/show.jl
Outdated
str = string( | ||
realref(z); | ||
digits, | ||
more, | ||
no_radius, | ||
condense, | ||
unicode, | ||
remove_trailing_zeros, | ||
) | ||
|
||
if !iszero(imagref(z)) | ||
str *= | ||
" + " * | ||
string( | ||
imagref(z); | ||
digits, | ||
more, | ||
no_radius, | ||
condense, | ||
unicode, | ||
remove_trailing_zeros, | ||
) * | ||
"im" | ||
end |
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.
str = string( | |
realref(z); | |
digits, | |
more, | |
no_radius, | |
condense, | |
unicode, | |
remove_trailing_zeros, | |
) | |
if !iszero(imagref(z)) | |
str *= | |
" + " * | |
string( | |
imagref(z); | |
digits, | |
more, | |
no_radius, | |
condense, | |
unicode, | |
remove_trailing_zeros, | |
) * | |
"im" | |
end | |
kwargs = ( | |
:digits => digits, | |
:more => more, | |
:no_radius => no_radius, | |
:condense => condense, | |
:unicode => unicode, | |
:remove_trailing_zeros => remove_trailing_zeros | |
) | |
str = string(realref(z); kwargs...) | |
if !iszero(imagref(z)) | |
str *= " + " * string(imagref(z); kwargs...) * "im" | |
end | |
return str |
seems a bit more readable for me
sure, we can release I see all of those changes as clear improvements! I wouldn't add any docstring to |
I have made changes according to your comments and set the version to 1.2. Regarding the kwargs I believe you already learned about this once before #152 (review). |
well clearly I forgot again 🤣 |
This PR makes several updates to the
show
andstring
methods forMag
,Arf
,Arb
andAcb
. Some of them are strict improvements, some of them are more opinionated. We can see which one we keep in the end.After:
Before:
(
Acb
just prints the real and imaginary parts using theArb
version)The first commit is an update to the interface and the remaining 5 make changes to the output format.
The
string_nice
method is removed, it never actually worked forMag
andArf
and was not particularly easy to use forArb
andAcb
since you had to contruct theflag
argument yourself. The interface is now based onBase.string
, with flags for the various options. In addition to this we now use thearf_get_str
function, which didn't exist when this code was first written. Before we convertedArf
toBigFloat
for printing, which is slightly problematic sinceArf
can represent more values thanBigFloat
.The changes to the output format are (in the same order as the commits):
Mag
by converting toArf
. I think this is to prefer as the old output format is not particularly informative.show
toMag
andArf
which only prints 6 digits. This is similar to howFloat64
andBigFloat
works.unicode
argument toBase.string
forArb
andAcb
. This doesreplace(str, "+/-" => "±", "..." => "…")
on the output.Arb
andAcb
. This sets the argumentscondense = 2
andunicode = true
for constructing the string.Base._prettify_bigfloat
method does. I find this very convenient when the values are small integers, as1.0
is significantly easier to read than1.0000000000000000000000000000000000000000000000000000000000000000000000000000
.Some issues/possible improvements:
Base.string
says that one should usually not definestring
directly, but this was the most natural way I found to be able to give formatting arguments when constructing strings. But Base does define it directly for e.g.Integer
andBigFloat
.Base.string
. Since the methods do accept a fairly large number of arguments this would maybe be reasonable to do.Mag
does round upwards. The printed result is hence not guaranteed to be an upper bound. I think it would be a nice improvement if it did round upwards. I thought I would add an issue in Flint about this (note that there is not even anymag_get_str
function in Flint).