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

Document recursive (c)transpose #15795

Closed
wants to merge 1 commit into from

Conversation

schmrlng
Copy link
Contributor

@schmrlng schmrlng commented Apr 7, 2016

Notes:

  • ctranspose{T<:AbstractVector}(::Type{T}) is defined indirectly through the methods here: https://github.com/JuliaLang/julia/blob/master/base/operators.jl#L261
  • As far as I can tell, transpose{T<:AbstractVector}(::Type{T}) = Matrix{eltype(T)} is the correct definition for all subtypes of AbstractVector.
  • According to a brief search, the jury's out on whether the julia community prefers American (behavior) or British (behaviour) spellings, so I went with the good old USofA.

@simonster
Copy link
Member

This seems like an improvement, so I support merging, but what if you have a matrix of eltype Union{Vector{T},S}?

@JeffBezanson
Copy link
Sponsor Member

...depends on #4774. I didn't realize transpose is defined on types. That's weird.

@simonster
Copy link
Member

@JeffBezanson I don't think it really was, until this PR. It's just that there's a fallback transpose(x) = x

@simonster
Copy link
Member

I suppose one issue with defining it on types is that you'd have fill(Vector{Int}, 2, 2)' == fill(Matrix{Int}, 2, 2). Not sure why you'd have a matrix of types, but it could happen...

@schmrlng
Copy link
Contributor Author

schmrlng commented Apr 8, 2016

@simonster I suppose in that case a solution would be to extend the definition of transpose on types to operate through unions, but I agree that transpose on types is a bit weird to extend more generally beyond this hack. The behavior in this PR could be achieved by defining a special transposetype function with

transposetype(x) = x
transposetype{T<:AbstractVector}(::Type{T}) = Matrix{eltype(T)}
transposetype(x::Union) = Union{(transposetype(t) for t in x.types)...} 

to be used in the allocations of B in transpose and ctranspose.

As for whether or not we want fill(Vector{Int}, 2, 2)' == fill(Matrix{Int}, 2, 2), to me this feels a lot like the original issue in #15512 of whether transpose means more-or-less permutedims or something mathematical like deeptranspose (or some other option from #4774).

@bjarthur
Copy link
Contributor

documentation is always good for an RC. i'd suggest merging this before 0.5-RC1

@tkelman
Copy link
Contributor

tkelman commented Jul 17, 2016

needs a rebase, and some related behavior was changed with the now deprecated fallback

@schmrlng
Copy link
Contributor Author

I haven't looked at this in ages but I've seen some rumblings recently about a return_type function? Something like that would probably be the right way to implement the element type issue with recursive transpose. This is a bit reminiscent of inference in #16622 although the problem here (#15512) is much more of a special case (i.e., the hacky solution transpose{T<:AbstractVector}(::Type{T}) = Matrix{eltype(T)} seemed sufficient, at least in terms of functionality).

Alternatively, we can let #15512 stand and I can make a PR with only the doc changes.

@bjarthur
Copy link
Contributor

a PR with just the docs would be great. transpose being recursive was a surprise to me, and one shouldn't have to go digging in the issue tracker to learn it's supposed to be that way.

@schmrlng schmrlng changed the title Document recursive (c)transpose and fix #15512 Document recursive (c)transpose Jul 19, 2016
@schmrlng
Copy link
Contributor Author

Okay, this is just the doc fix portion of the original PR. I guess fixing #15512 can wait until #4774 is figured out.

@Keno
Copy link
Member

Keno commented Dec 30, 2020

The rebased version of this (#24891) was declared obsolete, so I assume this is as well.

@Keno Keno closed this Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants