Skip to content

Conversation

@dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Jun 11, 2021

I believe these are relicts from the transition to lazy Adjoint and Transpose wrappers back in the v0.7/v1.0 times, perhaps artifacts of a mechanical translation of previous methods. Types à la Adjoint{...,Hermitian}, however, cannot be constructed from (the recommended) calls of tranpose and adjoint. They can only be constructed via brute force by calling the view constructors (with capital initial letters) directly. That is, in turn, not recommended, because it avoids the multiple dispatch mechanism built into adjoint, which may or may not return an Adjoint view, depending on types. The same holds, BTW, for adjoints/transposes of triangular matrices (EDIT: and diagonal matrices). In summary, because these wrapped type combinations should not occur in the wild, I don't expect any "slow fallback" issue to arise, but we should check, of course. IIRC, though, in our benchmark suite we may be testing still for Transpose/Adjoint(A), which should be changed if necessary. Independently from that, a pkgeval run should reveal if packages still relies on these methods.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Jun 11, 2021
@dkarrasch
Copy link
Member Author

Tests pass locally, and I think they do because multiplication of some brutally constructed Adjoint(Hermitian(...)) falls back to generic_matmul, which is perhaps hard to detect from pkgeval runs etc. The issue is that the first set of methods causes ambiguities, for whose fix we need another 20+ methods, not counting ambiguities caused in downstream packages, cf. JeffFessler/LinearMapsAA.jl#25.

@dkarrasch
Copy link
Member Author

Another option to detect issues in packages is to make all of the currently removed methods throw an error, run pkgeval and see what happens. I'll push some smaller test adjustments next.

@dkarrasch
Copy link
Member Author

@nanosoldier runbenchmarks("linalg", vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @christopher-dG

@dkarrasch dkarrasch changed the title Remove many * methods for symmetric/hermitian Remove many * methods for AdjOrTrans of sym/herm/diag/triangular Jun 14, 2021
@dkarrasch
Copy link
Member Author

This is ready for review. @andreasnoack @tkf?

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Great to get rid of all these unused methods. Once the error message has been expanded then this should be good to go.

@dkarrasch dkarrasch changed the title Remove many * methods for AdjOrTrans of sym/herm/diag/triangular Remove many */mul! methods for AdjOrTrans of sym/herm/diag/triangular Jun 16, 2021
@dkarrasch dkarrasch merged commit 6dfa690 into master Jun 17, 2021
@dkarrasch dkarrasch deleted the dk/cleanup branch June 17, 2021 08:56
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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.

4 participants