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.

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

Should we allow IOContext to accept kwargs #47396

Open
m-wells opened this issue Oct 31, 2022 · 18 comments
Open

Should we allow IOContext to accept kwargs #47396

m-wells opened this issue Oct 31, 2022 · 18 comments
Labels
display and printing Aesthetics and correctness of printed representations of objects. keyword arguments f(x; keyword=arguments) performance Must go faster

Comments

@m-wells
Copy link

m-wells commented Oct 31, 2022

IOContext already accepts Vararg{Pair{Symbol}}. The following code seems to work just fine.

> julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.8.2 (2022-09-29)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> Base.IOContext(io; kwargs...) = if length(kwargs) > 0
       IOContext(io, kwargs...)
       else
       convert(IOContext, io)
       end

julia> IOContext(stdout)
IOContext(Base.TTY(RawFD(13) open, 0 bytes waiting))

julia> io = IOContext(stdout)
IOContext(Base.TTY(RawFD(13) open, 0 bytes waiting))

julia> printstyled(io, rand())
0.39622936715782087
julia> io = IOContext(stdout; compact=true)
IOContext(Base.TTY(RawFD(13) open, 0 bytes waiting))

julia> printstyled(io, rand(), color=:red)
0.0857637

I can open a PR if there is interest.

@rfourquet
Copy link
Member

For context, the kwargs version was deleted in #23271 as being redundant.

@rfourquet rfourquet added the display and printing Aesthetics and correctness of printed representations of objects. label Oct 31, 2022
@m-wells
Copy link
Author

m-wells commented Oct 31, 2022

This kwargs version is actually faster. Also, passing options as key => value is not how most other Base methods accept options.

julia> IOContext(stdout, :compact=>true, :limit=>true, :color=>true);

julia> @time IOContext(stdout, :compact=>true, :limit=>true, :color=>true)
  0.000011 seconds (9 allocations: 304 bytes)
IOContext(Base.TTY(RawFD(13) open, 0 bytes waiting))

julia> # define the kwargs method

julia> IOContext(stdout; compact=true, limit=true, color=true);

julia> @time IOContext(stdout; compact=true, limit=true, color=true)
  0.000004 seconds (4 allocations: 128 bytes)
IOContext(Base.TTY(RawFD(13) open, 0 bytes waiting))

@m-wells
Copy link
Author

m-wells commented Nov 1, 2022

@rfourquet could we also label this as performance improvement as well?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 1, 2022

Dict constructors do not typically accept kwargs as values for initialization as an explicit decision, which is what this proposes adding for the special case of IOContext

@m-wells
Copy link
Author

m-wells commented Nov 2, 2022

Dict constructors do not typically accept kwargs as values for initialization as an explicit decision, which is what this proposes adding for the special case of IOContext

I see. It seems quite natural (and performant, see the example above) to allow this sort of construction. What is the rationale behind this convention.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 3, 2022

For Dict, it is because it fails to generalize appropriately, since it only meaningful for Symbol keys. And because it is how the AbstractDict itself is expected to be configured (if applicable). And because NamedTuple doesn't iterate to produce pairs, but rather is an ordered container of values closer to a Tuple (that last is a bit less relevant now that kwargs are once again acting like a list of Pairs objects and not like NamedTuples)

@LilithHafner
Copy link
Member

LilithHafner commented Nov 4, 2022

I think that the fact that IOContext is backed by a Dict is an implementation detail. To a user, its main role is mapping option names to values, for which keyword arguments are standard.

could we also label this as performance improvement as well?

The benchmarks you gave are not particularly convincing because the @time results are both just a few microseconds which is below level of noise/overhead in @time. However, with more accurate benchmarking, we can see that it is indeed a performance improvement :)

julia> using BenchmarkTools

julia> @btime IOContext(stdout)
  105.540 ns (1 allocation: 32 bytes)
IOContext(Base.TTY(RawFD(15) open, 0 bytes waiting))

julia> @btime IOContext(stdout, :compact=>true, :limit=>true, :color=>true)
  430.151 ns (9 allocations: 304 bytes)
IOContext(Base.TTY(RawFD(15) open, 0 bytes waiting))

julia> Base.IOContext(io; kwargs...) = if length(kwargs) > 0
           IOContext(io, kwargs...)
       else
           convert(IOContext, io)
       end

julia> @btime IOContext(stdout)
  105.931 ns (1 allocation: 32 bytes)
IOContext(Base.TTY(RawFD(15) open, 0 bytes waiting))

julia> @btime IOContext(stdout, :compact=>true, :limit=>true, :color=>true)
  424.854 ns (9 allocations: 304 bytes)
IOContext(Base.TTY(RawFD(15) open, 0 bytes waiting))

julia> @btime IOContext(stdout; compact=true, limit=true, color=true)
  117.416 ns (4 allocations: 128 bytes)
IOContext(Base.TTY(RawFD(15) open, 0 bytes waiting))

@LilithHafner LilithHafner added performance Must go faster keyword arguments f(x; keyword=arguments) labels Nov 4, 2022
@m-wells
Copy link
Author

m-wells commented Nov 4, 2022

This is probably a better definition than the one I had originally

@inline Base.IOContext(io; kw...) = isempty(kw) ? convert(IOContext, io) : IOContext(io, kw...)

@KristofferC
Copy link
Sponsor Member

I think that the fact that IOContext is backed by a Dict is an implementation detail.

The docs say:

In short, it is an immutable dictionary that is a subclass of IO.

so I am not sure it is really an implementation detail. And you interact with it like a dict with get etc.

@rfourquet
Copy link
Member

I would not base the decision for this on whether this is faster or not, as performance comparisons might evolve; unless the current performance with pairs is deemed problematic (but I don't recall seing complaints about it) and it's believed to not be easily improvable.
And yes IOContext is literally a container for pairs, as dicts, whereas usually keywords are more used to tweak how the method should proceed. But I recognize that kwargs syntax can be more convenient at times.

@m-wells
Copy link
Author

m-wells commented Nov 4, 2022

Ok, so maybe leave IOContext alone.
But how about something like sprint.
The kw... would take precedence over the context.
Currently it looks like

function sprint(f::Function, args...; context=nothing, sizehint::Integer=0)
    s = IOBuffer(sizehint=sizehint)
    if context isa Tuple
        f(IOContext(s, context...), args...)
    elseif context !== nothing
        f(IOContext(s, context), args...)
    else
        f(s, args...)
    end
    String(resize!(s.data, s.size))
end

It could be extended to

function sprint(f::Function, args...; context=nothing, sizehint::Integer=0, kw...)
    s = IOBuffer(sizehint=sizehint)
    if context isa Tuple
        ioc = IOContext(s, context...)
    elseif context !== nothing
        ioc = IOContext(s, context)
    else
        ioc = IOContext(s)
    end
    f(IOContext(ioc, kw...), args...)
    String(resize!(s.data, s.size))
end

@rfourquet
Copy link
Member

Why not, but kw... could be believed to be kwargs to be passed to f instead of to the IOContext constructor.

@m-wells
Copy link
Author

m-wells commented Nov 4, 2022

Fair point.
Maybe something like this.
This also has the benefit of providing additional functionality to sprint.

function sprint(f::Function, args...; context=nothing, sizehint::Integer=0, func_kws = (;), kws...)
    s = IOBuffer(sizehint=sizehint)
    if context isa Tuple
        ioc = IOContext(s, context...)
    elseif context !== nothing
        ioc = IOContext(s, context)
    else
        ioc = IOContext(s)
    end
    f(IOContext(ioc, kws...), args...; func_kws...)
    String(resize!(s.data, s.size))
end

@rfourquet
Copy link
Member

Still when seeing sprintf(f, args...; kws...), this pattern often means args and kwargs will be passed to f. But anyway, printing functions often don't require kwargs, so not sure that func_kws is needed here, and adding kws as you suggest is maybe fine. But kws would be redundant with context, so if kws is added, we should probably consider deprecating context, and also consider whether this should go together with adding kws to print, show and friends (adding this feature only to sprint feels ad hoc).

@m-wells
Copy link
Author

m-wells commented Nov 5, 2022

context would still be needed for passing an IOContext.

@o314
Copy link
Contributor

o314 commented Nov 14, 2022

New code here needs to be reviewed against https://docs.julialang.org/en/v1/manual/performance-tips/#Break-functions-into-multiple-definitions

norm(x::Vector) = sqrt(real(dot(x, x)))
norm(A::Matrix) = maximum(svdvals(A))

is better than

using LinearAlgebra

function mynorm(A)
    if isa(A, Vector)
        return sqrt(real(dot(A,A)))
    elseif isa(A, Matrix)
        return maximum(svdvals(A))
    else
        error("mynorm: invalid argument")
    end
end

BTW the couple namedtuple/kwargs gains maturity around julia v1.5 .
Afterwards, old vector pair / tuple stuff should be sunsetted and deprecated one day
(Does a kwargs break implies waiting for v2 ? Let's say ok...)

@m-wells
Copy link
Author

m-wells commented Nov 17, 2022

@o314 Julia doesn't dispatch on keyword arguments and args doesn't belong to sprint. I don't understand how to apply your comment in this scenario.

@o314
Copy link
Contributor

o314 commented Nov 17, 2022

Sorry for not being clear enough. The comment doesn't apply to the function head but to the function body (as in the example of the doc), and to your defense, flaws begun before your contribution. That's fine to fix them on next round anyway

function sprint(f::Function, args...; context=nothing, sizehint::Integer=0, func_kws = (;), kws...)
    iobuf = IOBuffer(; sizehint)
    ioctx = IOContext(iobuf, context; kws...)
    f(ioctx, args...; func_kws...)
    String(resize!(iobuf.data, iobuf.size))
end

IOContext(io::IO, tup::Tuple; kws...) = IOContext(io, tup..., kws...)
IOContext(io::IO, ::Nothing; kws...) = IOContext(io, kws...)
IOContext(io::IO, t::Any; kws...) = IOContext(io, t, kws...)

One can now compare

julia> sprint(print, rand(1:100, 10,3); context=:compact=>false, limit=true)
"[90 2 100; 49 14 4; … ; 40 100 88; 98 83 27]"

julia> sprint(print, rand(1:100, 10,3); context=:compact=>false, limit=false)
"[45 64 68; 37 13 4; 34 66 67; 44 20 30; 22 86 84; 38 20 84; 76 24 82; 65 29 64; 17 58 70; 64 28 95]"

That's small things but summed up, that naturally delivers a better message about what an iocontext ctor should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. keyword arguments f(x; keyword=arguments) performance Must go faster
Projects
None yet
Development

No branches or pull requests

6 participants