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

Naive fix for #13171 #17075

Merged
merged 3 commits into from
Jul 10, 2016
Merged

Naive fix for #13171 #17075

merged 3 commits into from
Jul 10, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jun 23, 2016

Ref JuliaLang/LinearAlgebra.jl#257. This fix's simplicity has me convinced that I'm either missing something or made a mistake.

@tkelman tkelman added the linear algebra Linear algebra label Jun 24, 2016
@@ -293,7 +293,7 @@ fldmod1{T<:Real}(x::T, y::T) = (fld1(x,y), mod1(x,y))
fldmod1{T<:Integer}(x::T, y::T) = (fld1(x,y), mod1(x,y))

# transpose
transpose(x) = x
transpose(x) = error("transpose not implemented for $(typeof(x)). Consider permutedims.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an error and not something like ArgumentError? I thought we were trying to move away from error()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason beyond mimicking

ctranspose(a::AbstractArray) = error("ctranspose not implemented for $(typeof(a)). Consider adding parentheses, e.g. A*(B*C') instead of A*B*C' to avoid explicit calculation of the transposed matrix.")
transpose(a::AbstractArray) = error("transpose not implemented for $(typeof(a)). Consider adding parentheses, e.g. A*(B*C.') instead of A*B*C' to avoid explicit calculation of the transposed matrix.")
. Should I change it to ArgumentError, and if so should I change the linked definitions to ArgumentErrors as well? Thanks for reviewing! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this PR self-contained for now - you can always open a PR for those others later. But yeah, I think something like ArgumentError is more useful than error().

Copy link
Member Author

Choose a reason for hiding this comment

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

Cheers, revised to throw an ArgumentError. Thanks!

@ivarne
Copy link
Member

ivarne commented Jul 4, 2016

Ideally it should be a MethodError (right?) But as long as we don't have #7512, we can't print the message in MethodError.

@andreasnoack
Copy link
Member

I think packages will hit this more often. @JeffBezanson was also not happy about adding a lot of no-op (c)transpose methods including the cases Char and AbstractString. The question is therefore if we should go all the way and declare transpose separate from permutedims and require that arrays of strings or chars use permutedims.

As a follow up on the last comment, I'll mention that I'm trying to rebase my transpose immutable branch and realized that the Transpose immutable can't be restricted to wrap AbstractMatrix and can't be a subtype of AbstractArray either since people will definately want to do A'x for their custom linear operators that are not subtypes of AbstractMatrix. In that sense, we are also on the path to separating transpose from permutedims where the latter only deals with AbstractArrays.

@Sacha0
Copy link
Member Author

Sacha0 commented Jul 7, 2016

I think packages will hit this more often. @JeffBezanson was also not happy about adding a lot of no-op (c)transpose methods including the cases Char and AbstractString. The question is therefore if we should go all the way and declare transpose separate from permutedims and require that arrays of strings or chars use permutedims.

As a follow up on the last comment, I'll mention that I'm trying to rebase my transpose immutable branch and realized that the Transpose immutable can't be restricted to wrap AbstractMatrix and can't be a subtype of AbstractArray either since people will definately want to do A'x for their custom linear operators that are not subtypes of AbstractMatrix. In that sense, we are also on the path to separating transpose from permutedims where the latter only deals with AbstractArrays.

+1 to decoupling speciously coupled concepts!

@andreasnoack
Copy link
Member

Let's then try to remove the definitions for AbstractString and Char and adjust the tests.

Sacha0 added 2 commits July 7, 2016 21:12
…ew missing `transpose` methods and correct a test that failed for lack of a `transpose` method.
@Sacha0
Copy link
Member Author

Sacha0 commented Jul 8, 2016

Let's then try to remove the definitions for AbstractString and Char and adjust the tests.

Done. Thoughts? Thanks and best!

@andreasnoack
Copy link
Member

Looks like datafmt.jl also needs some adjustments.

@Sacha0
Copy link
Member Author

Sacha0 commented Jul 8, 2016

Looks like datafmt.jl also needs some adjustments.

Points for CI. What say you now, Travis?

@andreasnoack andreasnoack merged commit 4a84310 into JuliaLang:master Jul 10, 2016
@tkelman
Copy link
Contributor

tkelman commented Jul 10, 2016

This needs to be a deprecation, not an immediate error.

@Sacha0
Copy link
Member Author

Sacha0 commented Jul 11, 2016

Thanks for the review and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants