Remove adjoint method calling transpose #838
Conversation
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
db92051 to
8bcc100
Compare
|
Actually should probably first fix Hecke and any other of our packages to replace |
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
8bcc100 to
87d7746
Compare
|
Also the tests fail. |
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
87d7746 to
ce91242
Compare
|
Yeah, they failed because after I made this PR, some uses of the |
|
In this particular case, could you perhaps call the branches in the other repos with the same name as this one so we can see the CI pass across all the packages. What I also find useful is to put a link to this PR in the description of all the others. This just helps alert whoever merges them to consider the PR's together. That approach seems to be working ok for @thofma and me so far and seems to be relatively little effort. (You are probably already aware of all this already. But it can't hurt to mention it once.) |
|
Absolutely -- these two PRs simply predate that bit of tooling. Unfortunately, I think I'll have to close and replace one of the two PRs for that. I'll go with the one on Hecke, as I have to update it anyway. |
Codecov Report
@@ Coverage Diff @@
## master #838 +/- ##
==========================================
+ Coverage 82.39% 86.04% +3.65%
==========================================
Files 86 93 +7
Lines 20462 28295 +7833
==========================================
+ Hits 16860 24347 +7487
- Misses 3602 3948 +346
Continue to review full report at Codecov.
|
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
ce91242 to
a96fa15
Compare
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
a96fa15 to
c152fc2
Compare
|
I've now opened matching PRs in Nemo and Hecke: Nemocas/Nemo.jl#1162 and thofma/Hecke.jl#383 Assuming everything works, we may want to first apply all those changes replacing |
|
I'm not too bothered either way, but as you see fit. |
The former unfortunately stands for `adjoint`, i.e., conjugate transpose. This patch does *not* remove our `adjoint` methods, even though they are arguably wrong. But too much other code relies on them to do this carelessly.
In Julia, adjoint is defined to be conjugate transposition. There are rings (e.g. finite fields of even degree over their base fields) that admit a canonical involution and where the notion of conjugate transposition is used as well, and standard in the literature. As such, I think it is misleading and potentially dangerous to simply map `adjoint` to `transpose`. One drawback of removing it is that the notation `a'` won't work anymore for transposing a matrix.
fingolfin
force-pushed
the
mh/remove-adjoint
branch
from
c152fc2 to
5728a33
Compare
|
Somehow the downstream tests are failing. |
|
I am just gonna mark this breaking and merge soon together with the other breaking PR. |
In Julia, adjoint is defined to be conjugate transposition. There are rings
(e.g. finite fields of even degree over their base fields) that admit a
canonical involution and where the notion of conjugate transposition is
used as well, and standard in the literature.
As such, I think it is misleading and potentially dangerous to simply map
adjointtotranspose.One drawback of removing it is that the notation
a'won't work anymore fortransposing a matrix.
See also
thofma/Hecke.jl#234and the discussion there, resp. Nemocas/Nemo.jl#1162 and thofma/Hecke.jl#383This is probably a breaking change, so shouldn't be merged lightly.