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

Remove checkeltype from Adjoint and Transpose #25274

Closed
wants to merge 1 commit into from

Conversation

andyferris
Copy link
Member

Fixes JuliaLang/LinearAlgebra.jl#495 (and fixes #25271) and is a better design anyway.

The remaining (somewhat confusing) errors on ""' and [""]' are due to the current design of Adjoint, lowering of ', and so-on. I didn't want to mess with any of that at the moment while @Sacha0 is on such a roll, but my guess of one direction where this will end up is that we can reinstate adjoint and have a "user friendly" error message on adjoint(::Any) reminding users of permutedims. I'm sure there are other solutions too!

Note that this PR competes with #25272 and #25268.

@Sacha0
Copy link
Member

Sacha0 commented Dec 28, 2017

I prefer your existing design: Insofar as we can check sanity of these wrapped objects on construction (and immediately emit a clear error message if that check fails), doing so seems preferable to allowing construction of nonsensical objects (and throwing a potentially less-clear error downstream if/when getindex gets called on the result, which may not occur). Thoughts? :)

@andyferris
Copy link
Member Author

Ideally, I would like zero-overhead for these operations (I take adjoints of SVector{3} all the time, which was a no-op, and wouldn't want my geometry calculations to slow down). If we can do that without the pure annotation I'm OK... (honestly I didn't check this yet but I didn't love the checkeltype design - partly because it's easy to circumvent it (Expr(:new, ...) is just one way, there are others involving Refs, etc).

In either case we can add the ::T annotations to getindex. This should avoid breakage of the AbstractArray{T} contract.

(We can also just opt-in to adjoint and have a nice message for adjoint(::Any) to use permutedims, to make it user-friendly).

@Sacha0
Copy link
Member

Sacha0 commented Dec 28, 2017

Ideally, I would like zero-overhead for these operations (I take adjoints of SVector{3} all the time, which was a no-op, and wouldn't want my geometry calculations to slow down). If we can do that without the pure annotation I'm OK... (honestly I didn't check this yet

Hopefully the annotation was not necessary for compile-time evaluation of the check :).

@JeffBezanson
Copy link
Member

Bump.

@dkarrasch
Copy link
Member

Is this still of interest? I applied the same changes to the current master, and the only tests that fail are those where the Adjoint/Transpose constructors are called directly with mismatching types, rather than calling adjoint or transpose. I'd think package authors/users would overwrite adjoint and transpose and, in case they use the wrapper types, make sure the eltype determination is correct?

@vtjnash
Copy link
Member

vtjnash commented Apr 22, 2021

Yes, if you rebase this, I'll approve and merge

@DilumAluthge DilumAluthge deleted the ajf/no-pure-adjoint branch August 24, 2021 05:20
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.

unexpected stack overflow on ""' Segmentation fault when transposing a char
6 participants