JuliaString / Format.jl

A Julia package to provide C and Python-like formatting support
Other
40 stars 6 forks source link

Handling of `AbstractIrrational` #50

Closed hhaensel closed 4 years ago

hhaensel commented 4 years ago

Currently Format.jl fails to handle the printout of Irrationals such as pi.

This can be circumvented by defining a new DEFAULTFORMATTER for AbstractIrrational. I wondered, what a good standard print format might be

In this PR I propose to introduce a new type and class character 'v' for "variable" which is per default right-aligned, so that the result of pyfmt(FormatSpec("2v"), pi) (EDIT: as well as fmt(pi,2)) is " π"

Together with the StringLiterals.jl package this would make it possible to write f"\%(pi, 10)" to print a right-aligned symbol.

I understand that the introduction of a new character type breaks with the standard Python format, so I am a bit unsure, whether this is a good idea, or how it could be done better. So I'd be happy to receive some feedback here.

EDIT: two more comments ...

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.2%) to 95.826% when pulling 34feb56def91c1b2874002ec932bacc21699e046 on hhaensel:master into c4d2f61847a12ae6abd2f2849cb0320a2f2e3e30 on JuliaString:master.

codecov-io commented 4 years ago

Codecov Report

Merging #50 into master will decrease coverage by 0.39%. The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #50     +/-   ##
=========================================
- Coverage   96.52%   96.12%   -0.4%     
=========================================
  Files           6        6             
  Lines         489      516     +27     
=========================================
+ Hits          472      496     +24     
- Misses         17       20      +3
Impacted Files Coverage Δ
src/fmt.jl 94.44% <100%> (-4.05%) :arrow_down:
src/fmtspec.jl 95.38% <100%> (+1.83%) :arrow_up:
src/fmtcore.jl 93.63% <94.44%> (+0.1%) :arrow_up:

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 c4d2f61...34feb56. Read the comment docs.

ScottPJones commented 4 years ago

I agree that Irrationals should be handled, however, I don't believe that there should be a type for formatting displays of irrational. Rather, if an irrational is passed to one of the numeric formats, it should be displayed as the value, and to the s string format, as the symbolic name. The default would be either s or one of the numeric forms such as f or g. The other formatted output functions such as @printf, Formatting.fmt all work with irrationals, printing the symbolic name with s, or the numeric value with f.

hhaensel commented 4 years ago

Thanks for your comments. I understand that introducing a new character is a bit strange. The basic idea is still to make it possible to format also composed numeric types, such as complex numbers, physical constants, etc. I have pushed the idea a bit, currently without including test routines. But fmt() now works for complex numbers. I would be happy to receive feedback whether my idea looks strange to you or not. The concept is to try define the output routines for numbers such that each type can have a fmt_number() routine to which the correct formatting routine is passed as argument (see 'fmtcore.jl`)

It also works for PhysicalConstants :smile: P.S.: I have removed the 'v' character and replaced it by calling fmt_default!() but then reset! won't work for these types...

hhaensel commented 4 years ago

Just to give a small code example for PhysicalConstants:

using Format
using PhysicalConstants.CODATA2018, Unitful

import Format.default_spec
default_spec!(Unitful.AbstractQuantity, 'f')
default_spec(::Type{<:Unitful.AbstractQuantity}) = Format.DEFAULT_FORMATTERS[Unitful.AbstractQuantity]

function Format.fmt_Number(x::Unitful.AbstractQuantity, f::Function)
    io=IOBuffer()
    print(io, f(x.val))
    if !Unitful.isunitless(unit(x))
        Unitful.has_unit_spacing(unit(x)) && print(io," ")
        show(io, unit(x))
    end
    String(take!(io))
end

c_0 = SpeedOfLightInVacuum;
fmt(c_0, 30, 1)        #  "            299792458.0 m s^-1"
pyfmt("30e", c_0)   #  "           2.997925e+08 m s^-1"
hhaensel commented 4 years ago

Just a thought: It might be a good idea to introduce 'S' for symbols instead of 'v' for variables. Alternatively, one could also opt for 'N' for Number. Anyhow, I added some tests and did some code beautifying.

hhaensel commented 4 years ago

By the way, rational numbers come for free (and are also part of the tests), as they are Numbers

ScottPJones commented 4 years ago

I still don't believe that it's a good idea to add any characters, for the format strings that are meant to be C or Python compatible. I think using s for symbolic/string output, or one of d,f,e for numeric output, is sufficient to get whatever output one wants, and doesn't potentially cause future incompatibility problems.

hhaensel commented 4 years ago

The Union{} is needed in order to cover cases like Complex{Integer} or Rational which are all of type UnionAll.

hhaensel commented 4 years ago

I am ok with removing 'S'. Do you have a good idea for resetting right-aligned strings?

hhaensel commented 4 years ago

But then we can't set defaults for all Rationals  like I did in the test: fmt_default!(Rational, 'f', prec = 2) ...? Also enhancing the scope for other types when no abstract type is available is less comfortable...

Edit: removed history from mail answer

hhaensel commented 4 years ago

Well, I think I found a way. I am still not so familiar with Types...

Edit:

ScottPJones commented 4 years ago

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.

hhaensel commented 4 years ago

I think, it is strongly desirable, to have a common format for all Complex{Intxx} types. It is just the analog case to have a common format for all Integer types. The same is true for all Complex{Floatxx} types. So if you don't like the Union{} so much, why not chosing the internal Type{T} where T type?

ScottPJones commented 4 years ago

I've added a PR #51 to address some of your issues, without adding new format characters. It adds a lot more generality to the defaults for types, by allowing any keyword arguments to be used (and preserved for use by reset!), which allows for making numbers right-aligned.

I'd like to collaborate with you, to make sure that the functionality you want is added to Format (and the formatted strings), and make sure that your contributions are noted in the git log as well.

What things still need to be done, in your opinion, once PR #51 is merged? I agree that handling Complex numbers in a better way is needed, as well as other numeric types such as the ones you mentioned with units, but it is better to make changes in separate PRs, instead of putting too much in one single PR.

ScottPJones commented 4 years ago

One issue for formatted display of numbers like complex or the ones with units, is how the width and precision values should be used. For strings, the precision gives the maximum length, which should not be used in that fashion for numbers. The width needs to apply to the entire string, not each number separately (as in a Complex number)

hhaensel commented 4 years ago

Pleasure to work with you! Your PR covers a lot of things I wouldn't have written that fast.

One issue for formatted display of numbers like complex or the ones with units, is how the width and precision values should be used.

  • In my code I use the same semantics as for real numbers, just that the same formatting is used for both real and imaginary part. Complex{<:Integer} are formatted as Integers, so prec is not used. width is used, but for the whole resulting string. Complex{<:AbstractFloat} are formatted as Floats, so prec is used, whereas width is still not used. Complex{Rational} is formatted as Rationals in each of the parts. If I force a formatting, such as 'f' or 'e', fx = float(x) converts to the respective float type which is then appropriately formatted. This only works, if we give up type stability for fx. Otherwise we would have to check the resulting type manually, but I thought this would be the strength of Julia 😉.

For strings, the precision gives the maximum length, which should not be used in that fashion for numbers. The width needs to apply to the entire string, not each number separately (as in a Complex number)

  • If I execute fmt("Hello World!", 5), I still receive "Hello World!" and not "Hello" ... 🤔
hhaensel commented 4 years ago

I will close this PR and build a new one on top of #51

ScottPJones commented 4 years ago

If I execute fmt("Hello World!", 5), I still receive "Hello World!" and not "Hello"

This is why I was explaining that there are different formatting methods, which are inconsistent, i.e. format, fmt, cfmt pyfmt. pyfmt uses a Python-like format string (it's missing '%' and 'g'/'G') fmt uses the Python-like formatting code, using the default set up (this was part of Tom Breloff's PR#10 that was never merged into Formatting.jl, which is why I made the fork years ago. cfmt generates handlers using the built-in Julia @printf.

For example:

julia> @printf "%.5s" "Hello World!"
Hello
julia> v = "Hello World!" ; f"\%.5s(v)"
"Hello"

The Python style formatters ignore precision for strings, unlike the C style ones, i.e.:

julia> f"\{.5s}(v)"
"Hello World!"
hhaensel commented 4 years ago

O wow that is, indeed, an important piece of information that I was missing. That makes things more complicated. I will nevertheless slow down a bit due to personal reasons.