-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Remove eltype check in adjtrans #40567
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
Conversation
@propagate_inbounds getindex(v::AdjOrTransAbsVec, i::Int) = wrapperop(v)(v.parent[i-1+first(axes(v.parent)[1])]) | ||
@propagate_inbounds getindex(A::AdjOrTransAbsMat, i::Int, j::Int) = wrapperop(A)(A.parent[j, i]) | ||
@propagate_inbounds getindex(v::AdjOrTransAbsVec{T}, i::Int) where {T} = wrapperop(v)(v.parent[i-1+first(axes(v.parent)[1])])::T | ||
@propagate_inbounds getindex(A::AdjOrTransAbsMat{T}, i::Int, j::Int) where {T} = wrapperop(A)(A.parent[j, i])::T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also apply to the getindex methods on the next line?
@andyferris Do you still have an opinion on this? It's been a while. |
@dkarrasch for the co-authorship, you do it when you commit / rebase: https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors |
That helped me in another rebase PR. The next issue is that I can't find @andyferris's e-mail address. |
I found this a while ago, which usually works pretty well: https://www.sourcecon.com/how-to-find-almost-any-github-users-email-address/. I added the address to the PR description, so you should just need to squash merge. There really should be some official interface for this though... |
Co-authored-by: Andy Ferris <[email protected]>
54fd671
to
2a45741
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed these messages.
Looks like a decent simplification to me. I suppose that this is still a good idea?
Should we merge this? @Sacha0 any follow-up thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me! Let's live dangerously with marginally fewer safety checks! 🏴☠️
Thanks @dkarrasch |
Co-authored-by: Andy Ferris <[email protected]> Co-authored-by: Andy Ferris <[email protected]>
Co-authored-by: Andy Ferris <[email protected]> Co-authored-by: Andy Ferris <[email protected]>
Co-authored-by: Andy Ferris <[email protected]> Co-authored-by: Andy Ferris <[email protected]>
This is a manual "rebase" of #25274. It removes the eltype checks at construction, which leaves users who call the constructor directly with more responsibility of providing correct eltype.
Closes #25274.
Not sure how to correctly assign authorship here:
Co-authored-by: Andy Ferris [email protected]