Skip to content

Improve consistency in printing #189

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

Merged
merged 1 commit into from
Jul 23, 2020
Merged

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Jul 3, 2020

Fixes #188

Before (Julia v1.4.2)

julia> N8f24[1/3 0.2]
1×2 Array{N8f24,2} with eltype Normed{UInt32,24}:
 0.333333  0.2

julia> N8f24[1/3, 0.2]
2-element Array{N8f24,1} with eltype Normed{UInt32,24}:
 0.33333333N8f24
 0.2N8f24

julia> FixedPoint[N0f8(1/3) N0f16(1/3)]
1×2 Array{Error showing value of type Array{FixedPoint,2}:
ERROR: MethodError: no method matching typechar(::Type{FixedPoint})

julia> FixedPoint[N0f8(1/3), N0f16(1/3)]
2-element Array{Error showing value of type Array{FixedPoint,1}:
ERROR: MethodError: no method matching typechar(::Type{FixedPoint})

julia> Normed{UInt128,28}(1)
1.0N100f28

After (Julia v1.4.2)

julia> N8f24[1/3 0.2]
1×2 Array{N8f24,2} with eltype Normed{UInt32,24}:
 0.333333  0.2

julia> N8f24[1/3, 0.2]
2-element Array{N8f24,1} with eltype Normed{UInt32,24}:
 0.33333333
 0.2

julia> FixedPoint[N0f8(1/3) N0f16(1/3)]
1×2 Array{FixedPoint,2}:
 0.333  0.33333

julia> FixedPoint[N0f8(1/3), N0f16(1/3)]
2-element Array{FixedPoint,1}:
 0.333N0f8
 0.33333N0f16

julia> Normed{UInt128,28}(1)
Normed{UInt128,28}(1.0)

@kimikage kimikage added the breaking Major breaking change label Jul 3, 2020
@kimikage
Copy link
Collaborator Author

kimikage commented Jul 3, 2020

FYI

julia> Float32[1/3 0.2]
1×2 Array{Float32,2}:
 0.333333  0.2

julia> Float32[1/3, 0.2]
2-element Array{Float32,1}:
 0.33333334
 0.2

julia> AbstractFloat[Float16(1/3) Float32(1/3)]
1×2 Array{AbstractFloat,2}:
 0.3333  0.333333

julia> AbstractFloat[Float16(1/3), Float32(1/3)]
2-element Array{AbstractFloat,1}:
  Float16(0.3333)
 0.33333334f0

@kimikage
Copy link
Collaborator Author

kimikage commented Jul 4, 2020

FixedPointNumbers.showtype() had performance issues as well as functional issues.

Before

julia> @benchmark FixedPointNumbers.showtype(io, N0f8) # Julia v1.4.2
BenchmarkTools.Trial:
  memory estimate:  192 bytes
  allocs estimate:  4
  --------------
  minimum time:     266.134 ns (0.00% GC)
  median time:      276.998 ns (0.00% GC)
  mean time:        341.150 ns (2.18% GC)
  maximum time:     7.560 μs (92.47% GC)
  --------------
  samples:          10000
  evals/sample:     313

julia> @benchmark FixedPointNumbers.showtype(io, N0f8) # Julia v1.6.0-DEV
BenchmarkTools.Trial:
  memory estimate:  336 bytes
  allocs estimate:  6
  --------------
  minimum time:     441.414 ns (0.00% GC)
  median time:      466.667 ns (0.00% GC)
  mean time:        573.091 ns (11.50% GC)
  maximum time:     242.056 μs (99.73% GC)
  --------------
  samples:          10000
  evals/sample:     198

After

julia> @benchmark FixedPointNumbers.showtype(io, N0f8) # Julia v1.4.2
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     94.312 ns (0.00% GC)
  median time:      136.266 ns (0.00% GC)
  mean time:        130.076 ns (0.00% GC)
  maximum time:     33.543 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     932

julia> @benchmark FixedPointNumbers.showtype(io, N0f8) # Julia v1.6.0-DEV
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     139.721 ns (0.00% GC)
  median time:      142.276 ns (0.00% GC)
  mean time:        156.099 ns (0.00% GC)
  maximum time:     63.378 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     861

It's still slow but this PR (and upcoming changes in ColorTypes) can reduce the number of times where the type aliases are printed. Therefore, I will not optimize showtype with a string dictionary or something.

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #189 into master will decrease coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   89.80%   89.74%   -0.06%     
==========================================
  Files           6        6              
  Lines         461      478      +17     
==========================================
+ Hits          414      429      +15     
- Misses         47       49       +2     
Impacted Files Coverage Δ
src/utilities.jl 96.00% <ø> (ø)
src/FixedPointNumbers.jl 87.69% <92.59%> (-0.03%) ⬇️
src/fixed.jl 90.26% <100.00%> (+0.08%) ⬆️

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 cab78d7...94b8747. Read the comment docs.

@kimikage kimikage force-pushed the issue188 branch 2 times, most recently from 323db62 to 38cc0ce Compare July 18, 2020 22:32
@kimikage kimikage marked this pull request as ready for review July 18, 2020 22:33
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Very nice change! Approved, small tweaks are optional.

function show(io::IO, x::FixedPoint{T,f}) where {T,f}
compact = get(io, :compact, false)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
compact = get(io, :compact, false)
compact = get(io, :compact, false)::Bool

There is no way for inference to know this, so it makes sense to help it.

It's not necessary for the :typeinfo access below because the only way you use it is with === and that's a builtin so there's only one "method."

Copy link
Collaborator Author

@kimikage kimikage Jul 21, 2020

Choose a reason for hiding this comment

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

BTW, in Base, the default value for :typeinfo seems to be set as Any, not nothing. So, I'll change it to Any.

print(io, "Array{")
showtype(io, X)
print(io, ",$(ndims(a))}")
toplevel && hasalias(X) && print(io, " with eltype ", X)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting to make this conditional on hasalias. Seems sensible.

@kimikage
Copy link
Collaborator Author

kimikage commented Jul 21, 2020

I'll make sure that v0.8.4 doesn't have any fatal side effects on the downstream packages, then I'll merge this.

Edit:
Although NanosoldierReports have been sick for the past few days, at least the tests of Images.jl was passed. So, I'll merge this and start v0.9 series.

@kimikage kimikage merged commit 55ee461 into JuliaMath:master Jul 23, 2020
@kimikage kimikage deleted the issue188 branch July 23, 2020 01:08
@kimikage kimikage mentioned this pull request Apr 30, 2024
38 tasks
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request Apr 30, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making display style consistent with Base
2 participants