export transpose! and ctranspose!

[Jutho]

This PR proposes to export transpose! and ctranspose!. When I was modified these functions a while ago ( #6690 ), I decided not to export them because the original transpose! was also not exported. However, given that a lot of the mutating methods are exported, it would make sense to export these as well.

If you agree to merge (before or after 0.3), I can also add them to the docs.

[JeffBezanson]

Maybe there should be an error if the two arguments are ===, since the function doesn't work in that case?

[ivarne]

Can't multiple arrays reference the same memory, so that you actually need to compare the pointers of both start and end of both arrays, to ensure they don't overlap?

[Jutho]

These are valid points. Is this correctly checked for all user exposed functions? I never looked into other examples, aside from permutedims!, where I know for certain that no such checks are in place. I always assumed that it was the user's responsibility to know what he was doing when using the ! functions. What if two strided arrays overlap, but in such a way that they do not reference the same memory locations? Suppose that for a 2N x 2N matrix you want to do transpose!(slice(A,1:2:2N,1:2:2N),slice(A,2:2:2N,2:2:2N)). Should this be allowed? If so, then it becomes pretty hard to check whether something is a valid operation or not. Maybe it's not too bad for transpose!, but for permutedims! this might be more difficult, since I don't think there is any simplification over comparing all memory locations.

[JeffBezanson]

I only brought this up since I tried transpose!(A) to see if I could transpose a single array in place. Seeing it only accepted 2 arguments, I tried transpose!(A,A).

[Jutho]

That would indeed be problematic with the current implementation. In place transposition is apparently a hard problem for non-square matrices. Note that this even changes the size of the matrix, which is something that you don’t typically expect for mutating operations on arrays.

On 02 Aug 2014, at 19:41, Jeff Bezanson [email protected] wrote:

I only brought this up since I tried transpose!(A) to see if I could transpose a single array in place. Seeing it only accepted 2 arguments, I tried transpose!(A,A).

—
Reply to this email directly or view it on GitHub.

[Jutho]

http://en.wikipedia.org/wiki/In-place_matrix_transposition

[JeffBezanson]

I agree you can only reasonably do it for square matrices, but this is just an example of something somebody might try.

[Jutho]

That’s true. I am totally open on how to deal with this. Only throw an error for this specific case explicitly, and warn against other potential issues in the docs. Or don’t check at all and warn in the docs.

On 02 Aug 2014, at 20:17, Jeff Bezanson [email protected] wrote:

I agree you can only reasonably do it for square matrices, but this is just an example of something somebody might try.

—
Reply to this email directly or view it on GitHub.

[ViralBShah]

I think we can merge this once the check is implemented. Throw an error for the specific case and warn against potential issues in docs seems like a reasonable solution.

[Jutho]

I rebased and added documentation, with explicit warning regarding unexpected results if the arrays overlap.

Regarding adding the check for the specific case where the two arguments are ===. I am happy to add them, but should I then be consistent, e.g. also add them in permutedims!, and maybe even copy! (although that's probably harmless), and the A_mul_B! family, and ...

There are many places where you can basically trigger this kind of problem. I am not sure whether we need to start adding checks now. If you use the mutating methods, aren't you supposed to know what you are doing.

[andreasnoack]

I don't think that these checks are necessary. If you are using the ! versions, you are supposed to know what you are doing.

[ViralBShah]

That's a fair point. Let's go ahead and merge.

[Jutho]

I've rebased and squashed the commits. If no more comments, I will merge tomorrow.

[ViralBShah]

I think the sparse transpose defines these in reverse order. Should we make it consistent?

[ViralBShah]

Also, I think these are useful to export, since there are cases where you can significantly avoid memory allocation.

[Jutho]

I think the sparse transpose defines these in reverse order. Should we make it consistent?

There doesn’t seem to be a transpose! for sparse matrices. I am not sure if that is what you mean?

[ViralBShah]

It exists. Look in csparse.jl.

[Jutho]

Now I see. I just tried with @which Base.transpose!(a,b) where a and b are two sparse matrices. The problem with this is that the transpose! method for sparse is completely disconnected (i.e. different function) from Base.transpose!. So that should also be fixed. I'll take a stab at it tonight.

I guess the argument of the sparse matrices is in conflict with the typical convention of the first argument being the one that is modified / used as output (e.g. in copy!, permutedims!, ...)

[ViralBShah]

That's right. It needs to be updated with the new convention.

[Jutho]

I've added the sparse functionality. Should I squash those two commits?

[ViralBShah]

I think it is better to have the commits the way they are, and a separate commit for the sparse transpose for future reference.

[Jutho]

That's what I thought.

[Jutho]

I think this is ok for @JuliaBackports

[tkelman]

thanks - second commit here doesn't apply cleanly, see conflict diff at https://gist.github.com/aeb8cc230b506e921a84
what's the best resolution there?

[tkelman]

there was a different PR for sparse in-place sorting, that I guess this one depends on due to the conflicts. maybe wait until after 0.3.3, look at backporting that one, then come back to this.

[Jutho]

That's fine with me. I know very little about the sparse functionality, I never use it and never really looked into it.

[JeffBezanson]

I'm not sure adding new exports really needs to be or should be backported.

[Jutho]

Also fine, no preference here.

[ViralBShah]

I think this one does not need backporting - see my other comment on not backporting the sparse multiplication update. Taking off the backport label.