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

RFC: print Array{T,1} as Vector{T} and Array{T,2} as Matrix{T} #36032

Closed
wants to merge 1 commit into from

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented May 26, 2020

Personally I find this clearer, especially when types are nested.

As an example

1-element Array{Union{X28004, Array{T,1} where T},1}:\n X(Any[X(1)])

vs

1-element Vector{Union{X28004, Vector{T} where T}}:\n X(Any[X(1)])

Some potential objections:

  • How about the other array types, AbstractVector, DenseVector etc? Could also be special cased imo.
  • Could potentially hide that Vector{T} is just another name for Array{T, 1}. - I feel this is something you learn pretty quickly and the convenience in printing makes up for the slight "pedagogical" issue.

@KristofferC KristofferC added display and printing Aesthetics and correctness of printed representations of objects. triage This should be discussed on a triage call labels May 26, 2020
@KristofferC
Copy link
Member Author

Statistics fail the doctests. @nalimilan, I'm not sure it is feasible to have Statistics live outside this repo but still run the CI / doctests for it. Too much friction.

@tkf
Copy link
Member

tkf commented May 27, 2020

  • How about the other array types, AbstractVector, DenseVector etc? Could also be special cased imo.

Can this be generalized to user-defined types? How about

@typealias MyVector{T} = MyArray{T,1}

for writing const = MyVector{T} = MyArray{T,1} and generating (a very safe implementation of) show(::IO, ::Type{<:MyVector})?

@nalimilan
Copy link
Member

Statistics fail the doctests. @nalimilan, I'm not sure it is feasible to have Statistics live outside this repo but still run the CI / doctests for it. Too much friction.

Yes, I guess we can point people to (a stable variant of) https://julialang.github.io/Statistics.jl/dev/ instead.

@mbauman
Copy link
Member

mbauman commented May 27, 2020

The pedagogical reservation is something I struggled with in #31149, too. To help alleviate it, I made sure the docs for StridedArray prominently pointed out that it's a Union. Looks like we already have that in the Vector and Matrix docstrings, but all these special type printings still feel pretty opaque to me as it becomes impossible to get the REPL to display the actual definition. What if we limited them (all) to specific IOContexts, special cased from summary and method tables?

@@ -588,12 +588,16 @@ end

function show_datatype(io::IO, x::DataType)
istuple = x.name === Tuple.name
isarray = x.name === Array{TypeVar(:T), TypeVar(:N)}.name
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
isarray = x.name === Array{TypeVar(:T), TypeVar(:N)}.name
isarray = x.name === Array.body.body.name

@mbauman
Copy link
Member

mbauman commented May 28, 2020

Triage was pretty much in favor — the two reservations were:

  • It's gonna break a lot of doctests.
  • It'd be nice to have an "escape hatch" to get the actual type printed out... somehow.

If we're going to break the doctests, it may be worth doing a more comprehensive solution all at once — searching the type's module for (potentially exported) names that define aliases and printing out the most specific ones. @vtjnash will try generalizing it in this manner, which will make it more useful and provide a bit more of a reward to packages.

@mbauman mbauman removed the triage This should be discussed on a triage call label May 28, 2020
@StefanKarpinski
Copy link
Member

  • It'd be nice to have an "escape hatch" to get the actual type printed out... somehow.

Perhaps an IOContext parameter that disables this?

@nalimilan
Copy link
Member

Would :compact => true make sense for this?

@tkf
Copy link
Member

tkf commented May 28, 2020

Can we have something like

"""
    show_typealias(io::IO, T::Type)

Try printing type alias for `T`.
"""
show_typealias(::IO, ::Type) = nothing

function show(io::IO, @nospecialize(x::Type))
    if get(io, :typealias, true)
        alias = try
            sprint(show_typealias; context = io)
        catch err
            msg = sprint(context = IOContext(io, :typealias => false)) do buf
                println(buf, "ERROR: show_typealias(::IO, ::")
                show_type(buf, x)
                println(buf, ") failed")
                showerror(buf, err)
            end
            println(stderr, msg)
            ()
        end
        if !isempty(alias)
            print(io, alias)
            return
        end
    end
    show_type(io, x)
end

function show_type(io::IO, @nospecialize(x::Type))
    # current show(::IO, ::Type) implementation
end

? (It can be made safer and more efficient but I think it's enough for a sketch.)

@JeffBezanson
Copy link
Member

I think we should merge this since (1) everyone seems to agree on this subset of changes, (2) we might as well get all those test and doc updates in.

@KristofferC
Copy link
Member Author

The annoying part is that this breaks doctest for Statistics which is not in this repo :/ Not sure how to deal with it. Turn off the doctests for Statistics?

@KristofferC
Copy link
Member Author

cc @nalimilan

@nalimilan
Copy link
Member

Yeah I already commented above. If that's possible, turning off doctests for Statistics should be OK since we run them in Statistics.jl anyway. Otherwise the section the manual could point to https://julialang.github.io/Statistics.jl/dev/.

@vtjnash
Copy link
Member

vtjnash commented Jun 15, 2020

Yeah, I have the same problem over at #36107. I made the necessary changes locally just for testing. We could push those upstream, then merge this with the new commit hash. Or figure out how to stop running them. I'm inclined towards the former?

I think we should merge this since (1) everyone seems to agree on this subset of changes, (2) we might as well get all those test and doc updates in.

Pros: we think this subset is good, and it'd directly reduce the diff of future possible changes. Cons: if we're going to break a lot of doctests (with this, and then again with (#361007), might be worth doing it all at once, and not twice.

@KristofferC
Copy link
Member Author

#36107

@KristofferC KristofferC closed this Jul 2, 2020
@DilumAluthge DilumAluthge deleted the kc/matrix_vector branch March 25, 2021 21:57
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants