Hh/general numbers #52

hhaensel · 2019-12-17T12:51:56Z

Allow for Complex, Rational and user-defined Numbers, e.g. unitful quantities.
Superseding #50 on top of #51

ScottPJones

Still changes that need to be addressed, esp. with the _pfmt* code.

src/fmt.jl

ScottPJones · 2019-12-17T17:55:37Z

src/fmt.jl

@@ -79,6 +79,9 @@ default_spec(::Type{<:AbstractString}) = DEFAULT_FORMATTERS[AbstractString]
 default_spec(::Type{<:AbstractChar})   = DEFAULT_FORMATTERS[AbstractChar]
 default_spec(::Type{<:AbstractIrrational}) = DEFAULT_FORMATTERS[AbstractIrrational]
 default_spec(::Type{<:Number})         = DEFAULT_FORMATTERS[Number]
+default_spec(::Complex{T} where T<:Integer)       = DEFAULT_FORMATTERS[Complex{T} where T<:Integer]


This should have Type{<:Complex} to handle complex numbers that use other numeric types as a fallback.
I still don't think you should have specific defaults for more specific types, but if so, they should be written as following:
These should be Type{Complex{<:Integer}}, Type{Complex{<:AbstractFloat}}, Type{Complex{<:Rational}}

I had tried exactly that before, but I received

julia> hh(x::Type{Complex{<:Integer}}) = show(typeof(x)) hh (generic function with 1 method) julia> hh(2 + 3im) ERROR: MethodError: no method matching hh(::Complex{Int64}) Closest candidates are: hh(::Type{Complex{#s12} where #s12<:Integer}) at REPL[114]:1 Stacktrace: [1] top-level scope at REPL[115]:1 julia> gg(x::Complex{<:Integer}) = show(typeof(x)) gg (generic function with 1 method) julia> gg(2 + 3im) Complex{Int64}

So I accepted that Type{Complex{<:Integer}} is a type of a type, which is what we don't want ...
I am happy to accept any prettier solution.

In much the same way, I think, the fallback should be <:Complex instead of Type{<:Complex}

julia> typeof(2 + im) <: Type{<:Complex} false julia> typeof(2 + im) <: Complex true

But I consider myself still a beginner in type programming, so please fell free to jump in, if you have a better way of putting it.

I wrote Complex{T} where T<:Integer because, strangely the Dict does not test on identity before adding a new type:

julia> d = Dict{Type{T} where T, String}() Dict{Type,String} with 0 entries julia> d[Complex{T} where T <: Integer] = "test" "test" julia> d[Complex{S} where S <: Integer] = "test" "test" julia> d[Complex{<:Integer}] = "test" "test" julia> d Dict{Type,String} with 3 entries: Complex{#s15} where #s15<:Integer => "test" Complex{T} where T<:Integer => "test" Complex{S} where S<:Integer => "test" julia> (Complex{T} where T<:Integer) == (Complex{S} where S<:Integer) true

So default_fmt!(Complex{<:Integer}, 's') will just add another entry to the dict instead of overwriting. That is also the reason, why - in the first run - I chose to define the type ComplexInteger = Complex{T} where T<:Integer.

src/fmt.jl

src/fmtspec.jl

test/fmt.jl

coveralls · 2019-12-17T23:30:40Z

Coverage increased (+0.6%) to 96.186% when pulling 0eb2a8c on hhaensel:hh/GeneralNumbers into 13b184d on JuliaString:master.

coveralls · 2019-12-17T23:30:41Z

Coverage increased (+0.03%) to 95.695% when pulling 874d042 on hhaensel:hh/GeneralNumbers into c4d2f61 on JuliaString:master.

codecov-io · 2019-12-17T23:40:00Z

Codecov Report

Merging #52 into master will decrease coverage by 0.05%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   95.79%   95.73%   -0.06%     
==========================================
  Files           6        6              
  Lines         499      539      +40     
==========================================
+ Hits          478      516      +38     
- Misses         21       23       +2

Impacted Files	Coverage Δ
src/fmt.jl	`95% <100%> (+1.57%)`	⬆️
src/formatexpr.jl	`100% <100%> (ø)`	⬆️
src/cformat.jl	`97.95% <100%> (+0.1%)`	⬆️
src/fmtspec.jl	`92.85% <81.81%> (-0.7%)`	⬇️
src/fmtcore.jl	`93.08% <90%> (-0.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13b184d...9e1d553. Read the comment docs.

hhaensel · 2019-12-18T00:08:28Z

oops, one commit was not sent. Coming shortly ...

ScottPJones · 2019-12-18T04:50:39Z

Project.toml

@@ -23,7 +23,7 @@ keywords = ["Strings", "Formatting"]
 license = "MIT"
 name = "Format"
 uuid = "1fa38f19-a742-5d3f-a2b9-30dd87b9d5f8"
-version = "1.0.1"
+version = "1.1.0"


You'll need to update the version as well, to "1.2.0"

ScottPJones · 2019-12-18T05:06:16Z

src/fmt.jl

+default_spec(::Type{<:AbstractIrrational}) = DEFAULT_FORMATTERS[AbstractIrrational]
+default_spec(::Type{<:Rational})       = DEFAULT_FORMATTERS[Rational]
+default_spec(::Type{<:Number})         = DEFAULT_FORMATTERS[Number]
+default_spec(::ComplexInteger)         = DEFAULT_FORMATTERS[ComplexInteger]


Again, you are confusing passing an instance of a particular type, with passing the type itself.

You need to have ::Type{<:Complex}.

I think it's best to work on making any complex type work at first, before trying to add the capability of having separate defaults for different parameterized Complex types.

ScottPJones · 2019-12-18T05:07:29Z

src/fmt.jl

+               (AbstractFloat,'f'),
+               (AbstractChar,'c'),
+               (AbstractString,'s'),
+               (ComplexInteger,'d'),


For now, please remove the separate ComplexInteger, ComplexFloat, ComplexRational default specs.

ScottPJones · 2019-12-18T05:11:46Z

src/fmtspec.jl

-        _pfmt_i(io, fs, ix, _Bin())
+        try
+            ix = Integer(x)
+            ty == 'd' || ty == 'n' ? _pfmt_i(io, fs, ix, _Dec()) :


Code smell here - shouldn't have a bunch of copy-paste.
The try should only be around the Integer conversion also.
Also, if the value cannot be converted to some Integer type, it should probably go directly to _pfmt_s.

I am afraid that I didn't completely get what you have in mind. We can't go directly to _pfmt_s, because we still need the formatting of the number part of the Number.

If we have a composed type like Complex{Int64} and we want to format it with 'd', this information needs to get passed on to the user-defined fmt_Number().

So my idea was:

we have _pfmt_i(out::IO, fs::FormatSpec, x::Integer, op::Op) where {Op} which does the normal formatting.

we add _pfmt_i(out::IO, fs::FormatSpec, x::Number, op::Op) where {Op} as a fallback which finally calls fmt_Number(x, f) where f contains _pfmt_i() with all the necessary parameters, so that the user simply needs to apply f() to the number part, e.g. f(real(x)).

Due to this fallback, I think, it is ok to accept ix in printfmt() to be either Integer or a number type with Integer number parts. Then we would get rid of the "code smell".

Example:
Integer(2.0) is 2.
Integer(2.0 + 3.0im) fails, so pyfmt("d", 2.0 + 3.0im) would fail. Therefore, fmt_Number() calls _pfmt_i(out, fs, x::Integer, op) on the real and imaginary part separately. So pyfmt("d", 2.0 + 3.0im) will finally result in 2 + 3im. If I would go directly to _pfmt_s, I would receive 2.0 + 3.0im.
Same story with _pfmt_f, but even more important, as e.g. float(2 - im) already acts on the number parts individually and results in 2.0 - 1.0im. So the type of fx in that case is always not predictable.

EDIT:
One comment on passing _pfmt_i in _pfmt_Number_i:
This would not be necessary, as the subtypes are covered by the op argument. whereas in the float case, where we have _pfmt_f, _pfmt_e and possibly later _pfmt_g. The subtype. Maybe, it's better to remove that.

For complex numbers, you could use the defaults for "d" based on the type T in Complex{T},
no need to have to define a potentially unlimited number of "ComplexXXXX" types, just for setting defaults.
Because I added the capability of storing keyword information along with the formats, that also could be used to set how particular types of complex numbers are printed.

For example: in the default dictionary, have simply an entry Complex,
and add support for a keyword that control whether integers should be printed as integers.
I don't think anything more would be necessary.

I'm looking for methods that don't just simply solve your problem for printing complex numbers, but also handle it in a more generic, extensible fashion.

One thing you may not have noticed, the C-style formatted strings (i.e. f"\%f(var)" don't use the pfmt* functions, they use cfmt, which generates a formatter using @printf, so it won't use the pfmt functions you've been modifying.

ScottPJones · 2019-12-18T05:16:37Z

src/fmtspec.jl

-        if isfinite(fx)
-            ty == 'f' || ty == 'F' ? _pfmt_f(io, fs, fx) :
-            ty == 'e' || ty == 'E' ? _pfmt_e(io, fs, fx) :
+        try


Same as above - if it can't get converted to something that is AbstractFloat, then it should output as string, or give an error.

ScottPJones · 2019-12-18T05:18:23Z

src/fmtcore.jl

@@ -262,3 +263,42 @@ function _pfmt_specialf(out::IO, fs::FormatSpec, x::AbstractFloat)
    end
 end

+function _pfmt_Number_f(out::IO, fs::FormatSpec, x::Number, _pf::Function)


These still have the issues that I mentioned before.

hhaensel · 2019-12-18T09:13:45Z

I must admit, that I am a bit lost, how you would like to have the code modified. I commented the parts where I have the most difficulties. Also, I don't know how to modify the type part where you said I am confusing passing an instance of a particular type, with passing the type itself.

For private reasons, I don't have the time to dig deeper. Would you mind, either accepting the PR and submitting your own corrections, or to submit a PR to my branch? I think that way will be faster and I will learn for my next PR ;-)

EDIT:
I changed the try catch in printfmt() to what I would consider a good version.

ScottPJones · 2019-12-18T16:01:02Z

I must admit, that I am a bit lost, how you would like to have the code modified. I commented the parts where I have the most difficulties.

For private reasons, I don't have the time to dig deeper.
I also have time issues (esp. with the vacation coming up), so it may be a week or so before I can do much,
but I will get back to it. I would like to both get this improved, and also help you learn more about dealing with types, etc.

I am very interested in getting this cleaned up - right now, there are about 4 separate code paths for formatting (all of which I inherited), which are rather inconsistent, and I dislike having the code depend on the code in Printf.jl also, using code generation.
Frankly, the code generation for each format string won't really speed things up for the formatted output, I feel the best thing is that the formatted output at compile time does do the parsing etc. of the format string, but then should generate a call to a specialized format function (but not use @eval etc.)

First off, before jumping into code changes, it might be good to write down a set of goals, for capabilities that you feel should be handled by an improved formatter, remembering that pyfmt, cfmt, fmt, and format all have rather different code paths and different capabilities (unfortunately).
Then, add a set of tests to show the output you feel they should produce (with @test_broken for now) (i.e. do test driven development).
Once that is done, I think we can better divvy up the work to really improve formatting for Julia. 😁

Thanks for your work and interest in improving formatting!

hhaensel · 2019-12-19T00:28:09Z

Sounds like a good idea. When I have some inspiration, I will let you know here ;-)

ScottPJones · 2019-12-19T15:02:34Z

I just checked with Python, and it's actually a bug that the Formatting (and my Format) packages don't truncate strings if a precision is specified, something you don't want to do for numeric formats (numeric formats should only round off based on prec, not chop off the end totally, that's also important for the Unitful types.

hhaensel · 2019-12-20T13:28:13Z

To bring this PR to a reasonable level, I have at least updated all formatting routines to use the same handling for general number types, i.e. calling fmt_Number(x, f) in case of an unsupported type. So fmt(), cfmt(), pyfmt and format can now handle complex numbers.
Currently, width is interpreted as measure for the full number string, whereas precision is interpreted as option for floating point numbers.
This makes it impossible to format with leading zeros for composed numeric types such as complex numbers, as this needs width information.

ScottPJones · 2019-12-20T13:56:57Z

To bring this PR to a reasonable level, I have at least updated all formatting routines to use the same handling for general number types, i.e. calling fmt_Number(x, f) in case of an unsupported type. So fmt(), cfmt(), pyfmt and format can now handle complex numbers.

Sounds good.
Have you tested it, or written tests, for cfmt, to make sure that is working correctly (since that actually calls the @printf code)?

Currently, width is interpreted as measure for the full number string, whereas precision is interpreted as option for floating point numbers.

That is as it should be.

This makes it impossible to format with leading zeros for composed numeric types such as complex numbers, as this needs width information.

I don't think that they even should use those sorts of formats options for composed numeric types.
Unless you have a good counter-example, I don't think that's that important.

ScottPJones · 2019-12-20T13:57:39Z

I'll try to take a look at this further tomorrow night (I'm getting ready to drive to Florida for Xmas vacation right now!)

hhaensel · 2019-12-20T17:44:27Z

Now the tests are back. There is still a problem with setting and resetting [format defaults]. This is probably due to my misimplementation of the types ...

hhaensel · 2019-12-21T10:35:07Z

Well, I think I understand types a bit better now ...
Seems to work well, even Coverage didn't complain :-)
Enjoy your Xmas season in Florida!

…fmt("%c",'A')`

hhaensel added 3 commits December 17, 2019 13:37

Allow for Complex and Rationals and user types

d2dafff

type conversion for formatting methods

3c88392

printfmt returned true if suffix was empty

ed836bd

ScottPJones requested changes Dec 17, 2019

View reviewed changes

hhaensel added 3 commits December 17, 2019 22:18

correction in fmtspec and fmt test

4a958ee

Define constants for complex subtypes

7d43657

add Rationals and some code beautifying

874d042

hhaensel requested a review from ScottPJones December 17, 2019 23:50

remove type instability

cf67f99

ScottPJones requested changes Dec 18, 2019

View reviewed changes

hhaensel added 2 commits December 18, 2019 10:39

new try catch in printfmt

4479a8a

adapt to master updates

d07f241

hhaensel force-pushed the hh/GeneralNumbers branch from 93b960d to d07f241 Compare December 18, 2019 11:29

change version to 1.2.0

314a4fb

apply changes to format()

38e386a

ScottPJones and others added 2 commits December 20, 2019 01:29

Update tests

82b9b43

apply changes to cfmt() and add tests

befc01c

hhaensel force-pushed the hh/GeneralNumbers branch from 665bf32 to c9bb7b8 Compare December 20, 2019 01:13

resolve merge conflict

ab0bdac

hhaensel force-pushed the hh/GeneralNumbers branch from c9bb7b8 to ab0bdac Compare December 20, 2019 01:16

separator handling for general Number types

7598556

add cformat tests for complex numbers

0233e95

correct complex types and add tests

9e1d553

include Char in the types of cfmt in order to match calls like `c…

0eb2a8c

…fmt("%c",'A')`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hh/general numbers #52

Hh/general numbers #52

hhaensel commented Dec 17, 2019

ScottPJones left a comment

ScottPJones Dec 17, 2019 •

edited

Loading

hhaensel Dec 17, 2019

coveralls commented Dec 17, 2019 •

edited

Loading

coveralls commented Dec 17, 2019

codecov-io commented Dec 17, 2019 •

edited

Loading

hhaensel commented Dec 18, 2019

ScottPJones Dec 18, 2019

ScottPJones Dec 18, 2019

ScottPJones Dec 18, 2019

ScottPJones Dec 18, 2019

hhaensel Dec 18, 2019 •

edited

Loading

ScottPJones Dec 18, 2019 •

edited

Loading

ScottPJones Dec 18, 2019

ScottPJones Dec 18, 2019

ScottPJones Dec 18, 2019

hhaensel commented Dec 18, 2019 •

edited

Loading

ScottPJones commented Dec 18, 2019 •

edited

Loading

hhaensel commented Dec 19, 2019

ScottPJones commented Dec 19, 2019

hhaensel commented Dec 20, 2019

ScottPJones commented Dec 20, 2019

ScottPJones commented Dec 20, 2019

hhaensel commented Dec 20, 2019 •

edited

Loading

hhaensel commented Dec 21, 2019

Hh/general numbers #52

Are you sure you want to change the base?

Hh/general numbers #52

Conversation

hhaensel commented Dec 17, 2019

ScottPJones left a comment

Choose a reason for hiding this comment

ScottPJones Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 17, 2019 • edited Loading

coveralls commented Dec 17, 2019

codecov-io commented Dec 17, 2019 • edited Loading

Codecov Report

hhaensel commented Dec 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhaensel Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

ScottPJones Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhaensel commented Dec 18, 2019 • edited Loading

ScottPJones commented Dec 18, 2019 • edited Loading

hhaensel commented Dec 19, 2019

ScottPJones commented Dec 19, 2019

hhaensel commented Dec 20, 2019

ScottPJones commented Dec 20, 2019

ScottPJones commented Dec 20, 2019

hhaensel commented Dec 20, 2019 • edited Loading

hhaensel commented Dec 21, 2019

ScottPJones Dec 17, 2019 •

edited

Loading

coveralls commented Dec 17, 2019 •

edited

Loading

codecov-io commented Dec 17, 2019 •

edited

Loading

hhaensel Dec 18, 2019 •

edited

Loading

ScottPJones Dec 18, 2019 •

edited

Loading

hhaensel commented Dec 18, 2019 •

edited

Loading

ScottPJones commented Dec 18, 2019 •

edited

Loading

hhaensel commented Dec 20, 2019 •

edited

Loading