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

Fix#25242 #25268

Closed
wants to merge 3 commits into from
Closed

Fix#25242 #25268

wants to merge 3 commits into from

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Dec 25, 2017

JuliaLang/LinearAlgebra.jl#495

A radical solution, removing the @pure-discussion, hopefully.
As a bonus, now arrays of Char, String, Symbol may be transposed.

@Sacha0
Copy link
Member

Sacha0 commented Dec 25, 2017

Much thanks for taking a stab at this @KlausC! :) I am not certain that removing the eltype consistency enforcement is wise. cc @andyferris

@Sacha0
Copy link
Member

Sacha0 commented Dec 25, 2017

Have you tried simply removing the problematic annotation and fixing any fallout by the way? If we are fortunate, there may be little or no fallout and the issue simply dissolves :).

@andyferris
Copy link
Member

andyferris commented Dec 25, 2017

Right, I think it is safer to remove checkeltype and rather put some type assertions on getindex. I think this might look something like:

@propagate_inbounds getindex(v::AdjOrTransAbsVec{T}, i::Int) where {T} = wrappertype(v)(v.parent[i])::T

As a bonus, now arrays of Char, String, Symbol may be transposed.

It's common to put changes like this as a separate PR, separate to the bugfix.

There is a "fix" for strings, characters, symbols AND arbitrary user-defined data types merged in v0.7 such that permutedims(["abc", "def"]) now should work, leaving adjoint and transpose for purely linear algebra purposes.

@KlausC
Copy link
Contributor Author

KlausC commented Dec 26, 2017

@Sacha0 @andyferris @JeffBezanson I am thinking, the stack overflow was a consequence of the inappropriate use of @pure checkeltype. But I also think we are only curing a symptom of improper design, which allows to call Adjoint for non-linalg objects.
After just removing the pure-annotation Adjoint("") does not cause a stack overflow, but it creates an object, which looks wrong. It does not support many useful operations, even show errors out.
Therefore I thought a better fix would be to assign (more) meaningful results for the element types Char, String, and Symbol (in addition to the removal of @pure.
Actually that is not my favourite solution, which would disallow the call of Adjoint/Transpose for non-linalg objects. Unfortunately there is no easy way of identifying those by type (e.g. common superclass).

@KlausC
Copy link
Contributor Author

KlausC commented Jan 25, 2018

Has been resolved by another PR

@KlausC KlausC closed this Jan 25, 2018
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.

3 participants