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 adjoint method calling transpose #234

Closed
wants to merge 1 commit into from

Conversation

fingolfin
Copy link
Contributor

In Julia, adjoint is conjugate transposition these days... And that also makes some sense in our settings, e.g. for matrices over finite fields with involution. (I am not saying we should use adjoint for that, just that we should at least avoid potential confusion...)

In Julia, adjoint is conjugate transposition these days
@thofma
Copy link
Owner

thofma commented Mar 24, 2021

This exists because ' lowers to adjoint:

julia> code_lowered() do x
         x'
       end
1-element Array{Core.CodeInfo,1}:
 CodeInfo(
1%1 = Main.:var"'"(x)
└──      return %1
)

julia> Main.var"'"
adjoint (generic function with 41 methods)

julia> Main.var"'" === adjoint
true

So your proposal is basically to remove ' for matrices in the Oscar ecosystem and write transpose everywhere instead, right?

PS: The ' notation for adjoint is also useless for us (unless we live in a world of finite fields), since the morphism needs to be specified.

@fieker
Copy link
Collaborator

fieker commented Mar 24, 2021

I guess we need to be careful:

  • for fmpz/fmpq there is no ambiguity
  • for nf_elem there might
  • similar, for ff
    I was (wrongly) working under the assumption that ' = transpose only. I don't know if this used to be the case or not.

But yes, since ' is not transpose, we need to be more precise

@fingolfin
Copy link
Contributor Author

To be honest, I didn't really think about the use of the ' operator at all. Personally, I don't like this syntax at all, and would indeed prefer if it is didn't exist in the first place, but of course numerics people will strongly disagree. But that did not enter my thought process at all, and while I personally dislike it, I realize this is probably just me being a grumpy old man; in any case, never intended to suggest people should not use it if they feel it is a good idea for their code.

But I do mind that we redefine adjoint to mean transpose -- that seems really dangerous to me, even if we restrict it to our own types. Perhaps if we only did it for types where the two are the same anyway... but for finite fields or number fields, it seems problematic.

So, I guess If that means we have to write transpose(a) instead of a', then that's how it is -- but I am also ok with tr(a) or something else (yes, we elsewhere agreed to use more verbose names, but my point here is that this is not the point here at all ;-).

BTW, as you probably are aware, there are efforts on the Julia side to allow other shorthands similar to a' for related operations such as transpose. They added it in JuliaLang/julia#38062 but that was reverted in JuliaLang/julia#40075 (a wise call IMHO, better to mull this over some more before it's put into stone). But it involved a unicode character, so it isn't relevant for us right now anyway...

@thofma
Copy link
Owner

thofma commented Mar 30, 2021

The problem is the usual numeric bubble in which julia was/is developed. In a world where not everything is a complex number, ' for adjoint and 'weird unicode character I cannot even type for transpose does not make much sense. I guess we have to remove this. So transpose everywhere it is.

P.S.: tr(a) is already the trace (defined in Base).

@wbhart
Copy link
Contributor

wbhart commented Apr 9, 2021

I had no idea ' was for adjoint. Very strange indeed. But if that is the case, I see no other possibility but to remove our use of it and adjoint.

I'd suggest we define ^T except that we use T everywhere as a type variable. What a shame.

@wbhart
Copy link
Contributor

wbhart commented Apr 9, 2021

a^:T is a possibility

@fingolfin
Copy link
Contributor Author

Closing this in favor of PR #383

@fingolfin fingolfin closed this Aug 18, 2021
@fingolfin fingolfin deleted the mh/adjoint branch March 31, 2023 17:26
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.

4 participants