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.

Sign up for GitHub

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

Handling of AbstractIrrational #50

Closed
wants to merge 8 commits into from
Closed

Handling of AbstractIrrational #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2a77eac
Handling `AbstractIrrational`: add a new number type 'v' (variable), …
hhaensel Dec 11, 2019
e19db39
add tests for type 'v' (e.g. AbstractIrrational)
hhaensel Dec 11, 2019
bfac4e1
add print method for Irrationals for verions < 1.2
hhaensel Dec 11, 2019
67ad47f
Introduction of abstract `Number`s
hhaensel Dec 15, 2019
ddbc981
include complex numbers and allow for any other type to be formatted
hhaensel Dec 15, 2019
643dc79
Add some tests for `Real` and `Complex`
hhaensel Dec 16, 2019
f33aba0
Replace 'v' by 'S', add some tests, code beautifying
hhaensel Dec 16, 2019
34feb56
Adding a default type for `Rational` and `ComplexRational` plus some …
hhaensel Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/fmt.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ mutable struct DefaultSpec
DefaultSpec(c::AbstractChar) = new(Char(c), FormatSpec(c))
end

const DEFAULT_FORMATTERS = Dict{DataType, DefaultSpec}()
const DEFAULT_FORMATTERS = Dict{Union{DataType, UnionAll}, DefaultSpec}()

# adds a new default formatter for this type
default_spec!(::Type{T}, c::AbstractChar) where {T} =
Expand All @@ -29,7 +29,12 @@ default_spec!(::Type{T}, ::Type{K}) where {T,K} =
(DEFAULT_FORMATTERS[T] = DEFAULT_FORMATTERS[K]; nothing)

# seed it with some basic default formatters
for (t, c) in [(Integer,'d'), (AbstractFloat,'f'), (AbstractChar,'c'), (AbstractString,'s')]
ComplexInteger = Complex{T} where T<:Integer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want a subtype here? Why not just all anything of type Complex?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted automatic Integer formatting for all Complex{Integer} types in one go, that's why I changed the dict type tp Union{DataType,UnionAll}.
So I can write fmt_default!(Complex{T} where T<:Integer} and also default_spec!(Complex{T} where T<:Integer}, 'f')
Otherwise we would need to to this for all subtypes individually.
The same hold for any other package that has a similar type system without an abstract supertype.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this means is that you have to set up defaults for pretty much any arbitrary Complex number, which I don't think is desirable behavior.

ComplexFloat = Complex{T} where T<:AbstractFloat
ComplexRational = Complex{T} where T<:Rational
for (t, c) in [(Integer,'d'), (AbstractFloat,'f'), (AbstractChar,'c'), (AbstractString,'s'),
(ComplexInteger,'d'), (ComplexFloat,'f'), (Number,'S'), (AbstractIrrational,'S'),
(Rational,'S'), (ComplexRational,'S')]
default_spec!(t, c)
end

Expand Down Expand Up @@ -62,10 +67,16 @@ end
# methods to get the current default objects
# note: if you want to set a default for an abstract type (i.e. AbstractFloat)
# you'll need to extend this method like here:
default_spec(::Type{<:Integer}) = DEFAULT_FORMATTERS[Integer]
default_spec(::Type{<:AbstractFloat}) = DEFAULT_FORMATTERS[AbstractFloat]
default_spec(::Type{<:AbstractString}) = DEFAULT_FORMATTERS[AbstractString]
default_spec(::Type{<:AbstractChar}) = DEFAULT_FORMATTERS[AbstractChar]
default_spec(::Type{<:Integer}) = DEFAULT_FORMATTERS[Integer]
default_spec(::Type{<:AbstractFloat}) = DEFAULT_FORMATTERS[AbstractFloat]
default_spec(::Type{<:AbstractString}) = DEFAULT_FORMATTERS[AbstractString]
default_spec(::Type{<:AbstractChar}) = DEFAULT_FORMATTERS[AbstractChar]
default_spec(::Type{<:AbstractIrrational}) = DEFAULT_FORMATTERS[AbstractIrrational]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the one for Number is needed.

default_spec(::Type{<:Number}) = DEFAULT_FORMATTERS[Number]
default_spec(::ComplexInteger) = DEFAULT_FORMATTERS[ComplexInteger]
default_spec(::ComplexFloat) = DEFAULT_FORMATTERS[ComplexFloat]
default_spec(::Rational) = DEFAULT_FORMATTERS[Rational]
default_spec(::ComplexRational) = DEFAULT_FORMATTERS[ComplexRational]

default_spec(::Type{T}) where {T} =
get(DEFAULT_FORMATTERS, T) do
Expand Down Expand Up @@ -176,7 +187,7 @@ function fmt end
# TODO: do more caching to optimize repeated calls

# creates a new FormatSpec by overriding the defaults and passes it to pyfmt
# note: adding kwargs is only appropriate for one-off formatting.
# note: adding kwargs is only appropriate for one-off formatting.
# normally it will be much faster to change the fmt_default formatting as needed
function fmt(x; kwargs...)
fspec = fmt_default(x)
Expand All @@ -200,3 +211,6 @@ function fmt(x, syms::Symbol...; kwargs...)
d = _add_kwargs_from_symbols(kwargs, syms...)
fmt(x; d...)
end

#fmt_default!(AbstractIrrational, 's', :right)
#fmt_default!(Number, 's', :right)
38 changes: 38 additions & 0 deletions src/fmtcore.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# core formatting functions
export fmt_Number

### auxiliary functions

Expand Down Expand Up @@ -262,3 +263,40 @@ function _pfmt_specialf(out::IO, fs::FormatSpec, x::AbstractFloat)
end
end

function _pfmt_Number_f(out::IO, fs::FormatSpec, x::Number, _pf::Function)
fsi = FormatSpec(fs, width = -1)
f = x::AbstractFloat->begin
io = IOBuffer()
_pf(io, fsi, x)
String(take!(io))
end
s = fmt_Number(x, f)
_pfmt_s(out, fs, s)
end

function _pfmt_Number_i(out::IO, fs::FormatSpec, x::Number, op::Op, _pf::Function) where {Op}
fsi = FormatSpec(fs, width = -1)
f = x::Integer->begin
io = IOBuffer()
_pf(io, fsi, x, op)
String(take!(io))
end
s = fmt_Number(x, f)
_pfmt_s(out, fs, s)
end

function _pfmt_i(out::IO, fs::FormatSpec, x::Number, op::Op) where {Op}
_pfmt_Number_i(out, fs, x, op, _pfmt_i)
end

function _pfmt_f(out::IO, fs::FormatSpec, x::Number)
_pfmt_Number_f(out, fs, x, _pfmt_f)
end

function _pfmt_e(out::IO, fs::FormatSpec, x::Number)
_pfmt_Number_f(out, fs, x, _pfmt_e)
end

function fmt_Number(x::Complex, f::Function)
s = f(real(x)) * (imag(x) >= 0 ? " + " : " - ") * f(abs(imag(x))) * "im"
end
26 changes: 19 additions & 7 deletions src/fmtspec.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,26 @@
# width ::= <integer>
# prec ::= <integer>
# type ::= 'b' | 'c' | 'd' | 'e' | 'E' | 'f' | 'F' | 'g' | 'G' |
# 'n' | 'o' | 'x' | 'X' | 's'
# 'n' | 'o' | 'x' | 'X' | 's' | 'S'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't want any non-standard characters added, 's' works fine for outputting symbols already.

#
# Please refer to http://docs.python.org/2/library/string.html#formatspec
# for more details
#

## FormatSpec type

const _numtypchars = Set(['b', 'd', 'e', 'E', 'f', 'F', 'g', 'G', 'n', 'o', 'x', 'X'])
const _numtypchars = Set(['b', 'd', 'e', 'E', 'f', 'F', 'g', 'G', 'n', 'o', 'x', 'X', 'S'])

_tycls(c::AbstractChar) =
(c == 'd' || c == 'n' || c == 'b' || c == 'o' || c == 'x') ? 'i' :
(c == 'e' || c == 'f' || c == 'g') ? 'f' :
(c == 'c') ? 'c' :
(c == 's') ? 's' :
(c == 'S') ? 'S' :
error("Invalid type char $(c)")

struct FormatSpec
cls::Char # category: 'i' | 'f' | 'c' | 's'
cls::Char # category: 'i' | 'f' | 'c' | 's' | 'S'
typ::Char
fill::Char
align::Char
Expand Down Expand Up @@ -84,7 +85,7 @@ end

## parse FormatSpec from a string

const _spec_regex = r"^(.?[<>])?([ +-])?(#)?(\d+)?(,)?(.\d+)?([bcdeEfFgGnosxX])?$"
const _spec_regex = r"^(.?[<>])?([ +-])?(#)?(\d+)?(,)?(.\d+)?([bcdeEfFgGnosxXS])?$"

function FormatSpec(s::AbstractString)
# default spec
Expand Down Expand Up @@ -164,27 +165,38 @@ _srepr(x) = repr(x)
_srepr(x::AbstractString) = x
_srepr(x::AbstractChar) = string(x)
_srepr(x::Enum) = string(x)
@static if VERSION < v"1.2.0-DEV"
_srepr(x::Irrational{sym}) where {sym} = string(sym)
end

function printfmt(io::IO, fs::FormatSpec, x)
cls = fs.cls
ty = fs.typ
if cls == 'i'
ix = Integer(x)
ix = x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've introduced type instability here, and it's better to just get an exception if the format class isn't valid for the given type.

try
ix = Integer(x)
catch
end
ty == 'd' || ty == 'n' ? _pfmt_i(io, fs, ix, _Dec()) :
ty == 'x' ? _pfmt_i(io, fs, ix, _Hex()) :
ty == 'X' ? _pfmt_i(io, fs, ix, _HEX()) :
ty == 'o' ? _pfmt_i(io, fs, ix, _Oct()) :
_pfmt_i(io, fs, ix, _Bin())
elseif cls == 'f'
fx = float(x)
fx = x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, better for this to get an exception, instead of slowing this down by introducing type instability, and covering up what might be useful information for the programmer.

Copy link
Author

@hhaensel hhaensel Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not so sure whether we should care about type stability here. (I am definitely not an expert here!)
But packages like PhysicalConstants redefine float() to work on unitful quantities. So float(x) ends up to be something else than a subtype of AbstractFloat, e.g. float(c_0) is 2.99792458e8 m s^-1.
So if we keep fx = float(x) , fx will be type unstable anyhow.

Well, I think I found a way. I am still not so familiar with Types...
still struggling to formulate something similar like Complex{<:Integer} if I remove the Union{}

Why do you need to separate out different types of complex numbers for defaults?
For things like complex numbers, you'll need to be careful about how width / precision is handled,
so that it doesn't just get treated like a string after the initial conversion to a string (which it looks like your code might be doing), which might end up having critical information truncated.

I think, you misinterpreted my code. I have added new less specific methods, e.g. _pfmt_f(io, x::Number, ...), which are called if the currently existing method does not cover the type, i.e. if x is not a subtype of AbstractFloat in case of _pfmt_f(). The method calls the routine _pfmt_Number_f() which then calls fmt_Number(). (This method needs to be defined by the user, if he wants to add a new number type. For Complex types I have already defined it.)

The last argument of that method is an anonymous formatting routine for the specific type ('i', 'e' or 'f') where the width of the FormatSpec is set to -1. This should be mainly a copy of the Number's show() method where the value is replaced by f(value). The result type of the function needs to be a String which is then passed to _pfmt_s() with the original FormatSpec, which then takes into account the width of the resulting string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result type of the function needs to be a String which is then passed to _pfmt_s() with the original FormatSpec, which then takes into account the width of the resulting string.

That's the problem, because depending on the length, it may truncate important information.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see this: f"\{1.3f}(1+2im)" results in "1.000 + 2.000im"
_pfmt_s() only sets the minimal width.

try
fx = float(x)
catch
end
if isfinite(fx)
ty == 'f' || ty == 'F' ? _pfmt_f(io, fs, fx) :
ty == 'e' || ty == 'E' ? _pfmt_e(io, fs, fx) :
error("format for type g or G is not supported yet (use f or e instead).")
else
_pfmt_specialf(io, fs, fx)
end
elseif cls == 's'
elseif cls == 's' || cls == 'S'
_pfmt_s(io, fs, _srepr(x))
else # cls == 'c'
_pfmt_s(io, fs, Char(x))
Expand Down
21 changes: 19 additions & 2 deletions test/fmt.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,19 @@ i = 1234567
@test fmt(i) == "1234567"
@test fmt(i,:commas) == "1,234,567"

@test_throws ErrorException fmt_default(Real)
@test_throws ErrorException fmt_default(Complex)
@test fmt(2 - 3im, 10) == " 2 - 3im"
@test fmt(pi - 3im, 15, 2) == " 3.14 - 3.00im"

@test fmt(3//4, 10) == " 3//4"
@test fmt(1//2 + 6//2 * im, 15) == " 1//2 + 3//1*im"

fmt_default!(Rational, 'f', prec = 2)
fmt_default!(Format.ComplexRational, 'f', prec = 2)

@test fmt(3//4, 10, 2) == " 0.75"
@test fmt(3//4, 10, 1) == " 0.8"
@test fmt(1//2 + 6//2 * im, 23) == " 0.500000 + 3.000000im"
@test fmt(1//2 + 6//2 * im, 15, 2) == " 0.50 + 3.00im"

fmt_default!(Int, :commas, width = 12)
@test fmt(i) == " 1,234,567"
Expand All @@ -41,3 +52,9 @@ fmt_default!(UInt16, 'd', :commas)
fmt_default!(UInt32, UInt16, width=20)
@test fmt(0xfffff) == " 1,048,575"

v = pi
@test fmt(v) == "π"
@test fmt(v; width=10) == " π"

v = MathConstants.eulergamma
@test fmt(v, 10, 2) == " γ"
13 changes: 13 additions & 0 deletions test/fmtspec.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,3 +234,16 @@ end
@test pyfmt("*>5f", Inf) == "**Inf"
@test pyfmt("⋆>5f", Inf) == "⋆⋆Inf"
end

@testset "Format Symbols (S) for Irrationals" begin
@test pyfmt("10S", pi) == " π"
@test pyfmt("3S", MathConstants.eulergamma) == " γ"
@test pyfmt("10.2f", MathConstants.eulergamma) == " 0.58"
@test pyfmt("<3S", MathConstants.e) == "ℯ "
end

@testset "Format Symbols (S) for Irrationals" begin
pyfmt("10s", 3//4) == "3//4 "
pyfmt("10S", 3//4) == " 3//4"
pyfmt("10.1f", 3//4) == " 0.8"
end