Opened 10 years ago

Closed 2 years ago

#10501 closed task (fixed)

Deprecate adjoint in favor of adjugate

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-8.7
Component: linear algebra Keywords: notation, linear algebra, adjugate, matrices, determinants
Cc: was, mvngu, kohel, tornaria, mjo Merged in:
Authors: Kwankyu Lee Reviewers: Darij Grinberg
Report Upstream: N/A Work issues:
Branch: bf44252 (Commits, GitHub, GitLab) Commit: bf44252756d2f663acc479c1a345c1913716a1f6
Dependencies: Stopgaps:

Status badges

Description (last modified by klee)

Matrix methods named adjoint and _adjoint are renamed adjugate and _adjugate and replacements are added that raise deprecation warnings.

This is part of the program at #10465.

Attachments (1)

trac_10501-deprecate-adjoint.patch (11.6 KB) - added by rbeezer 10 years ago.

Download all attachments as: .zip

Change History (33)

Changed 10 years ago by rbeezer

comment:1 Changed 10 years ago by rbeezer

  • Cc was mvngu kohel tornaria added
  • Status changed from new to needs_review

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 follow-up: Changed 10 years ago by tornaria

  • Status changed from needs_review to needs_work

I already stated some objections to this on the mailing list, but I'll repeat:

On deprecating "adjoint" meaning "matrix of cofactors"

  1. it's standard terminology and has meant this in sage for long
  2. "adjugate" is newer and (IMO) less standard terminology -- in particular it has no obvious translations

On using "adjoint" meaning "conjugate transpose"

  1. "conjugate transpose" is easy to say, and it's really what is meant
  2. the "adjoint operator" for a matrix seems ill-defined, because a matrix is not an operator but only a representation of an operator in some basis.

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 in reply to: ↑ 2 Changed 10 years ago by rbeezer

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 conjugate_transpose(), which I find a really clumsy command, but workable in the interim. William has suggested a more general adjoint function, which I would need to think about some more, but maybe that does not help with any of your objections (sounds like maybe that is worse in your view).

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, Rob

comment:4 Changed 8 years ago by mjo

  • Cc mjo added

+1 from me. I hit this today, and just checked a handful of books:

  • Atkinson, An Introduction to Numerical Analysis, 1989. Section 7.1.
  • Axler, Linear Algebra Done Right, 1997. Ch. 6.
  • Marcus & Minc, Introduction to Linear Algebra, 1988. Section 1.4.
  • Meyer, Matrix Analysis and Applied Linear Algebra, 2000. Section 3.2.
  • Rudin, Functional Analysis, 1991. Chapter 4.
  • Shilov, Linear Algebra, 1977. Section 7.6.

All of which use the "new" meaning. In the interest of fairness, I also found,

  • Edwards, Elementary Linear Algebra, 2000. Section 3.4.

Which uses the cofactor definition.

comment:5 follow-up: Changed 8 years ago by jhpalmieri

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 A.adjoint() expecting the old meaning, and vice versa. Something like A.conjugate_transpose() can be found by tab completion; is that good enough? Is A.adjugate() the right name for the other version?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by mjo

Replying to jhpalmieri:

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 A.adjoint() expecting the old meaning, and vice versa. Something like A.conjugate_transpose() can be found by tab completion; is that good enough? Is A.adjugate() the right name for the other version?


Did someone seriously implement m.conjugate_transpose() as a shortcut for m.conjugate().transpose()? =)

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 adjoint, give it a new name, and then give something else the old name.

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 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by rbeezer

Replying to mjo:

Did someone seriously implement m.conjugate_transpose() as a shortcut for m.conjugate().transpose()? =)

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 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by kcrisman

Did someone seriously implement m.conjugate_transpose() as a shortcut for m.conjugate().transpose()? =)

It's not as bad as you think, because tab-completion doesn't work on m.conjugate(), though it would be awesome if Sage could magically know that...

Rob, so what does the latest version of your book do?

comment:9 in reply to: ↑ 8 Changed 8 years ago by rbeezer

Replying to kcrisman:

Rob, so what does the latest version of your book do?

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:10 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:12 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/10501
  • Commit set to 0669f64d384d8b42838d590a41a6bf9fcb115979
  • Milestone changed from sage-6.4 to sage-8.4
  • Priority changed from major to minor
  • Status changed from needs_work to needs_review

I want to revive this ticket. So here is the needed patch.

One thing not found in Rob's original patch is alias adjoint_classical of adjugate. The alias is used in quadratic form code in Sage.


New commits:

0669f64Deprecate adjoint for adjugate and adjoint_classical

comment:15 Changed 3 years ago by klee

  • Description modified (diff)

comment:16 Changed 3 years ago by git

  • Commit changed from 0669f64d384d8b42838d590a41a6bf9fcb115979 to 43ab6df581d54a2d627c030443a9320c6a7dee8e

Branch pushed to git repo; I updated commit sha1. New commits:

a61f618Merge branch 'develop'
1a2f6a0properly deprecate adjoint
43ab6dfpyflakes check and fix exception messages style

comment:17 Changed 3 years ago by git

  • Commit changed from 43ab6df581d54a2d627c030443a9320c6a7dee8e to 08eee094dd63965b8aadf3a38b2f3fe620910a9b

Branch pushed to git repo; I updated commit sha1. New commits:

08eee09import deprecated_function_alias on global level to avoid crash

comment:18 Changed 3 years ago by git

  • Commit changed from 08eee094dd63965b8aadf3a38b2f3fe620910a9b to d1bbfb1bd29c9cfb688d24d9a71e92ce98148513

Branch pushed to git repo; I updated commit sha1. New commits:

d1bbfb1change more adjoint to adjugate

comment:19 Changed 3 years ago by git

  • Commit changed from d1bbfb1bd29c9cfb688d24d9a71e92ce98148513 to 2f1597566c868739e748599aa5e8b2c2eb59775e

Branch pushed to git repo; I updated commit sha1. New commits:

2f15975fix neglected doctest failures

comment:20 Changed 3 years ago by klee

  • Milestone changed from sage-8.4 to sage-8.5

comment:21 Changed 2 years ago by gh-darijgr

  • Branch changed from u/klee/10501 to public/ticket/10501
  • Commit changed from 2f1597566c868739e748599aa5e8b2c2eb59775e to b4994c43854ebbece7186b7b7b5cd132873f9939
  • Keywords notation linear algebra adjugate matrices determinants added

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:

64b9491Merge branch 'u/klee/10501' of git://trac.sagemath.org/sage into adj
b4994c4actually define the adjugate in the doc

comment:22 Changed 2 years ago by git

  • Commit changed from b4994c43854ebbece7186b7b7b5cd132873f9939 to 356285df8e0ede0f6d2ccc02559744004528b744

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

356285dDeprecate adjoint for adjugate and adjoint_classical

comment:23 Changed 2 years ago by klee

Squashed and rebased to sage 8.6. In addition, refined some docstrings and comments.

comment:24 follow-up: Changed 2 years ago by kcrisman

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 in reply to: ↑ 24 ; follow-up: Changed 2 years ago by klee

Replying to kcrisman:

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).

Positive review on his part of the code. It is up to him to put his name to the Author field.

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.

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?

Also, don't we typically test deprecations (and then remove when deprecation is done)?

I didn't know. I don't know.

comment:26 in reply to: ↑ 25 Changed 2 years ago by kcrisman

Or do you regard the small changes as no improvements?

No, it's just that it causes more opportunities for clashes with other tickets.

Also, don't we typically test deprecations (and then remove when deprecation is done)?

I didn't know. I don't know.

For one of many examples, see (at least right now correct link) this spot

            sage: x, y, z = var('x, y, z')
            sage: S = CoordinatePatch((x, y, z)); S
            doctest:...: DeprecationWarning: Use Manifold instead.
            See http://trac.sagemath.org/24444 for details.
            Open subset of R^3 with coordinates x, y, z

I guess this should be done for positive review.

comment:27 Changed 2 years ago by gh-darijgr

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.

comment:28 Changed 2 years ago by git

  • Commit changed from 356285df8e0ede0f6d2ccc02559744004528b744 to bf44252756d2f663acc479c1a345c1913716a1f6

Branch pushed to git repo; I updated commit sha1. New commits:

bf44252Add a test for deprecation

comment:29 Changed 2 years ago by klee

Anything else to do?

comment:30 Changed 2 years ago by gh-darijgr

  • Milestone changed from sage-8.5 to sage-8.7
  • Reviewers set to Darij Grinberg

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.)

comment:31 Changed 2 years ago by klee

  • Status changed from needs_review to positive_review

comment:32 Changed 2 years ago by vbraun

  • Branch changed from public/ticket/10501 to bf44252756d2f663acc479c1a345c1913716a1f6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.