-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Deprecate adjoint in favor of adjugate #10501
Comments
Attachment: trac_10501-deprecate-adjoint.patch.gz |
comment:1
Three files caused doctest errors on a full run with only the necessary changes in sage/matrix. I've made changes in these other places to fix those failures, and the affected files now pass their tests. I'm running the full suite right now. I've cc'ed folks who I think might be able to double-check that no complications have crept in. If you want to sneak a quick look at the patch, here's a quick guide: Minh, David: sage/crypto/classical.py, inverse_key() for a Hill Cryptosystem Gonzalo: sage/quadratic_forms/quadratic_form_ternary_Tornaria.py, adjoints of a form William: sage/quadratic_forms/quadratic_form.py, adjoint_primitive() |
comment:2
I already stated some objections to this on the mailing list, but I'll repeat: On deprecating "adjoint" meaning "matrix of cofactors"
On using "adjoint" meaning "conjugate transpose" Moreover, if there are two colliding usages of the name "adjoint", I would find it more reasonable to keep the usage that is already traditional in Sage. The usage of "adjoint" is ubiquitous in relation to quadratic forms afaict (and, as John Cremona pointed out, is where the term originates with Gauss on ternary quadratic forms) Reference for "Adjoint of a matrix": Bourbaki, Elements, book 2, chapter III, section 11, exercise 9: The adjoint of a square matrix X of order n over A is the matrix X = (det (A'")) of minors of A" of order n — 1. (Note that the term also shows at the index of terminology of the book) PS: searching for "The adjoint of a square matrix" bourbaki in books.google.com, yields the above passage. |
comment:3
Replying to @tornaria: Hi Gonzalo, I certainly read your postings to the mailing list carefully and appreciated the points you raised. However, I had not realized you were so opposed to the change. After some discussion, I asked 'Is there any objection to deprecating the current .adjoint() function (which returns a matrix of cofactors) and renaming it as the "adjugate"?' It was not meant to be an official vote, but I got +1 replies from Grout, Cremona, Loeffler and Stein. Dima P and Karl Crisman had earlier voiced support. There were no objections stated once I asked the question carefully. So I have been proceeding on the assumption that there was strong support. I do not believe I changed any of the names of the commands for quadratic forms, though I can see that causing confusion if the adjoint of a matrix becomes the conjugate transpose. I have written a patch (#10471) with the I have twice now taught a "matrix analysis" course and it seems to me that adjoint gets used regularly (but maybe not consistently) for the conjugate transpose. I am in the middle of making a major push to add significant amount of Sage code to my introductory linear algebra text, which is going very nicely. But I need to also fix my "complex inner product" since I defined it with the conjugation on the "wrong" half. So I would really like to keep Sage, my text, and the word "adjoint" all consistent with each other when I get to that point in a few weeks. Do you have some suggestions for a way forward? Thanks, |
comment:4
+1 from me. I hit this today, and just checked a handful of books:
All of which use the "new" meaning. In the interest of fairness, I also found,
Which uses the cofactor definition. |
comment:5
Hmm. Given two completely different uses of the word "adjoint" in this situation, I wonder if the right solution is to avoid it completely (with a deprecation warning for a while). If we use the "new" meaning, there will still be people who type |
comment:6
Replying to @jhpalmieri:
Did someone seriously implement I never thought to look for another method, I just did the operations individually. From what I understand, the terms "adjoint" and "adjunct" come from higher algebra, most of which is over my head. If that's the case, books written after e.g. category theory became popular will probably gravitate towards the new terminology. Although it does suck to have to deprecate Most of us have access to math departments; maybe we could do a survey of people working in linear algebra? If the result is overwhelming, rename it. |
comment:7
Replying to @orlitzky:
Yep, that was me. ;-) But the BDFL suggested it. Required reading: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/YjImMWVVwo4 You will see a lot of support for changes. You'll see one conscientious objector. I dropped it. If someone else wants to carry the torch, I'll have their back. Rob |
comment:8
It's not as bad as you think, because tab-completion doesn't work on Rob, so what does the latest version of your book do? |
comment:9
Replying to @kcrisman:
conjugate-transpose has always been called "adjoint," in line with my experience teaching numerical linear algebra. I even have my inner product conjugating the correct vector now. ;-) See: http://linear.ups.edu/html/section-MO.html#subsection-AM I almost never have need to reference the matrix of cofactors (proposed as adjugate here), but do use it one exercise about building a matrix inverse this way. See: Exercise PDM.T20 in http://linear.ups.edu/html/section-PDM.html Rob |
comment:14
I want to revive this ticket. So here is the needed patch. One thing not found in Rob's original patch is alias New commits:
|
Branch: u/klee/10501 |
Author: Kwankyu Lee |
Commit: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:21
I wholeheartedly agree with "adjugate". When I see "adjoint", I look up the definition. When I see "adjugate", I immediately know what is meant. Pushed a little commit to improve the documentation. IMHO, this is an easy ticket to review: just run all doctests. If they work, then it's fine. New commits:
|
Changed branch from u/klee/10501 to public/ticket/10501 |
Changed keywords from none to notation, adjugate, matrices, determinants |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:23
Squashed and rebased to sage 8.6. In addition, refined some docstrings and comments. |
comment:24
If klee and darij are both agreed that each other's respective contributions are sufficient, then you can jointly set to positive review (sometimes Authors are 1, 2 and Reviewers are 2, 1). I do wonder whether some of the py3 things like - raise TypeError("Oops! The matrix must have " + str(n) + " rows. =(")
+ raise TypeError("the matrix must have {} rows".format(n)) are necessary on this ticket, since it makes it a little more of a patch bomb. Also, don't we typically test deprecations (and then remove when deprecation is done)? |
comment:25
Replying to @kcrisman:
Positive review on his part of the code. It is up to him to put his name to the Author field.
Not necessary. But I think we don't need to freak away from making small improvements unrelated with the main issue of the ticket. Do we? Or do you regard the small changes as no improvements?
I didn't know. I don't know. |
comment:26
No, it's just that it causes more opportunities for clashes with other tickets.
For one of many examples, see (at least right now correct link) this spot
I guess this should be done for positive review. |
comment:27
Sorry, I don't have time for this :/ I could reread the rebased branch once the stress from the semester start has subsided, but I'm generally extremely short on time until September or so(?). Sorry. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:29
Anything else to do? |
comment:30
The new version looks good to me. If complete doctests pass (anyone please check), please make this a positive_review. (I should not be listed as author; my changes were trivial.) |
Reviewer: Darij Grinberg |
Changed branch from public/ticket/10501 to |
Matrix methods named
adjoint
and_adjoint
are renamedadjugate
and_adjugate
and replacements are added that raise deprecation warnings.This is part of the program at #10465.
CC: @williamstein @sagetrac-mvngu @sagetrac-kohel @tornaria @orlitzky
Component: linear algebra
Keywords: notation, adjugate, matrices, determinants
Author: Kwankyu Lee
Branch/Commit:
bf44252
Reviewer: Darij Grinberg
Issue created by migration from https://trac.sagemath.org/ticket/10501
The text was updated successfully, but these errors were encountered: