#31940 closed enhancement (fixed)

Absolutely simple endomorphism rings

Reported by: gh-BarinderBanwait Owned by:
Priority: major Milestone: sage-9.5
Component: number theory Keywords: hyperelliptic curves, jacobians, endomorphism rings
Cc: roed, nbruin, edgarcosta, davide.lombardo@… Merged in:
Authors: Barinder S. Banwait, Davide Lombardo Reviewers: Edgar Costa, Nils Bruin
Report Upstream: N/A Work issues:
Branch: 462db22 (Commits, GitHub, GitLab) Commit: 462db22ebabae2134de4a3d4f156bb68b698e2d9
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-BarinderBanwait)

This commit adds the first functionality for endomorphism rings of jacobians of genus 2 curves over Q. There is a new class EndomorphismRing, and two methods of this class: is_absolutely_field which determines whether or not the endomorphism algebra is a field; and is_absolutely_trivial, which determines whether or not the ring is just the integer ring Z.

One corresponding attribute has been added to the Jacobian of the hyperelliptic curve: is_generic, which is a wrapper for is_absolutely_trivial above.

The algorithms are based on Lombardo's paper "Computing the geometric endomorphism ring of a genus 2 Jacobian".

This is needed for trac ticket #30837.

Change History (33)

comment:1 Changed 13 months ago by gh-BarinderBanwait

  • Branch set to u/gh-BarinderBanwait/absolutely_simple_endomorphism_rings

comment:2 Changed 13 months ago by gh-BarinderBanwait

  • Authors set to Barinder S. Banwait and Davide Lombardo
  • Commit set to a8a8a20832dcf8515186ef995f4e37b2a879a2a1
  • Description modified (diff)
  • Keywords hyperelliptic curves jacobians endomorphism rings added
  • Priority changed from major to blocker
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 13 months ago by gh-BarinderBanwait

  • Component changed from PLEASE CHANGE to number theory

comment:4 Changed 13 months ago by gh-BarinderBanwait

  • Description modified (diff)

comment:5 Changed 13 months ago by gh-BarinderBanwait

  • Authors changed from Barinder S. Banwait and Davide Lombardo to Barinder S. Banwait, Davide Lombardo

comment:6 Changed 13 months ago by nbruin

  • Priority changed from blocker to major

Blockers are for fixes without which it's unacceptable to ship a new release of sagemath. If a ticket is a dependency for another one, then you can just the other ticket to depend on it. That doesn't block releasing or working on the tickets; it just indicates that the dependency needs to be merged before the depending ticket. The dependent ticket should probably have its branch based on the dependency (which is one of the reasons to indicate the dependency on the ticket: knowing which order to merge branches in can save rebasing)

comment:7 Changed 13 months ago by gh-BarinderBanwait

Thanks for clarifying Nils -- I didn't realise 'blocker' meant 'release blocker', but in hindsight I should have guessed that. Apologies, and thanks for updating the dependency on the other ticket.

comment:8 Changed 13 months ago by gh-BarinderBanwait

  • Cc davide.lombardo@… added

comment:9 Changed 12 months ago by git

  • Commit changed from a8a8a20832dcf8515186ef995f4e37b2a879a2a1 to ad600bb293ffc07c3fbdd25679bc581993787e01

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

ad600bbFixed doc strings

comment:10 Changed 12 months ago by gh-BarinderBanwait

  • Status changed from new to needs_review

I've updated the docstrings on the two modules jacobian_endomorphisms.py and jacobian_generic.py, both of which pass when locally running those modules individually, so I'm marking this as ready for review. The linter is failing, but those failures don't seem related to these changes.

comment:11 Changed 12 months ago by gh-BarinderBanwait

  • Description modified (diff)

comment:12 Changed 12 months ago by edgarcosta

  • Cc edgarcosta added

I left some comments in: https://github.com/sagemath/sagetrac-mirror/pull/2/files and I'm happy to look at this again once the comments have been addressed.

comment:13 follow-ups: Changed 12 months ago by tscrim

Why doesn't EndomorphismRing inherit from Homset? It should also use _repr_ rather than __repr__.

I think you should not cache the result of endomorphism_ring. It should redirect to Hom, which does caching, and implement a _Hom_ method as

def _Hom_(self, Y, category):
    if self == Y:
        from .jacobian_endomorphisms import EndomorphismRing
        return EndomorphismRing(self)
    return super()._Hom_(Y, category)

That way someone doing End(X) or Hom(X, X) will get the endomorphism ring as well.

A few things about the documentation:

     def is_generic(self, B=200):
         r"""
-        Returns whether the absolute endomorphism algebra of self is the
+        Return whether the absolute endomorphism algebra of ``self`` is the
         integer ring `\ZZ`.
 
         INPUT:
 
-        - ``B`` -- the bound which appears in the statement of the algorithm
-        from Lombardo's paper. Defaults to 200.
+        - ``B`` -- (default: 200) the bound which appears in the statement of
+          the algorithm from [Lom2019]_
 
         OUTPUT:
 
-            (bool): boolean indicating whether or not the absolute endomorphism
-            ring is isomorphic to the integer ring.
+        Boolean indicating whether or not the absolute endomorphism
+        ring is isomorphic to the integer ring.
 
-        EXAMPLES::
+        EXAMPLES:
 
-        This is LMFDB curve 603.a.603.2
+        This is LMFDB curve 603.a.603.2::

             sage: R.<x> = QQ[]
             sage: f = 4*x^5 + x^4 - 4*x^3 + 2*x^2 + 4*x + 1
             sage: C = HyperellipticCurve(f)

and similar changes. The biggest is when EXAMPLES is followed by text, it should be a single colon :, when something it to have Sage code after it, it should be a double colon ::.

The Codes up Lombardo's Algorithm 4.15 is not a really good explanation of what the function does. It is good to state This is Algorithm 4.15 in [Lom2019]_., yet it is good to be more self-contained with a more descriptive version of what the function does.

comment:14 Changed 12 months ago by edgarcosta

Barinder emailed me:

I've been struggling to implement the lazy attributes you suggested, particularly in 'jacobian_endomorphisms.py' regarding the flags '_have_established_geometrically_field' and '_have_established_geometrically_trivial'. The problem is that these are not attributes in the conventional sense, and is related to the warning at the top of the file.

I agree with you. Perhaps, __endomorphism_ring is the only one that makes sense, see https://github.com/sagemath/sagetrac-mirror/pull/2/files#r669811139

With that in mind, I would add include a proof flag in the input of the methods is_absolutely_field, is_absolutely_trivial, and is_generic, and raise an error if proof=True and you would be returning False.

Note, I don't think you need to do this is_geom_trivial_when_field and get_is_geom_irred as these are not available to the user. However, I would say in documention string that any False returned by those functions is not proved.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 12 months ago by edgarcosta

Replying to tscrim:

Why doesn't EndomorphismRing inherit from Homset? It should also use _repr_ rather than __repr__.

I don't think makes sense for this ticket. The ticket is not providing the ring in any shape or form, indeed, this ticket only provides a method to check if End = ZZ, and if not we cannot even prove it with the current method.

comment:16 Changed 12 months ago by git

  • Commit changed from ad600bb293ffc07c3fbdd25679bc581993787e01 to b7c94ea8c830e265861c69601ee19e2699eb4791

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

b7c94eaaddress review comments

comment:17 in reply to: ↑ 13 Changed 12 months ago by gh-BarinderBanwait

@tscrim: Thank you very much for your comments, most of which I have implemented in the latest commit; however as Edgar suggested I haven't added the inheritance of Homset to EndomorphismRing, and have used the lazy_attribute for caching which Edgar suggested.

comment:18 Changed 12 months ago by git

  • Commit changed from b7c94ea8c830e265861c69601ee19e2699eb4791 to e5fb6e0cac6bef8801a84ef27f5de81f05971a8b

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

e5fb6e0fix failing tests

comment:19 in reply to: ↑ 15 ; follow-up: Changed 12 months ago by tscrim

Replying to edgarcosta:

Replying to tscrim:

Why doesn't EndomorphismRing inherit from Homset? It should also use _repr_ rather than __repr__.

I don't think makes sense for this ticket. The ticket is not providing the ring in any shape or form, indeed, this ticket only provides a method to check if End = ZZ, and if not we cannot even prove it with the current method.

It is a Homset, and it will provide consistency with other such things within Sage. There isn't anything more you have to implement on top of it. Moreover, it will be much more naturally accessible via the Hom functor, which would handle the caching, and easy to extend to be an actual homset. No ring structure needs to be implemented for this. However, if you don't think it is for the best, then this will be fine. It is just outside of the framework of what we do elsewhere in Sage.

@gh-BarinderBanwait? Please at least rename __repr__ -> _repr_ so it will work with the rename() method. This is standard for things inheriting from SageObject.

One other minor point `False` -> ``False``.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 12 months ago by nbruin

Replying to tscrim:

Replying to edgarcosta:

Replying to tscrim:

Why doesn't EndomorphismRing inherit from Homset? It should also use _repr_ rather than __repr__.

I don't think makes sense for this ticket. The ticket is not providing the ring in any shape or form, indeed, this ticket only provides a method to check if End = ZZ, and if not we cannot even prove it with the current method.

It sounds to me that this ticket shouldn't even be implementing a method "endomorphism_ring" at all. That slot should be reserved for a routine that actually implements "endomorphism ring" and returns a ring. I'm not so sure that Homset is the appropriate place to hook in to, unless we actually can make it act on the Jacobian and/or divisor class group of the curve. We have infrastructure in place that can already give the endomorphism ring (unproven) in terms of matrices, and therefore with ring structure, in the form of C.riemann_surface(prec=...).endomorphism_basis() -- and I think Edgar has some pretty good ideas for speeding this up considerably for hyperelliptic curves (it would involve implementing some of the other integration schemes than Gauss-Legendre)

It sounds like, for now, it should perhaps provide a method endomorphism_ring_is_ZZ on genus 2 curves or their jacobians to provide access. If you're not actually implementing endomorphism rings, you shouldn't be forcing design decisions on them: you'll just make it difficult for the next person who might.

Looking at the code, it look like it has some further functionality, but as far as I can see, these are True/False? checks. I think it's false advertising to hang these things under an "endomorphism_ring" attribute. They are much better as true/false checks on the original object.

The implementation already follows the pattern where the non-trivial work happens in stand-alone functions. Just get those merged and make them available. The appropriate interface can be worked out afterwards. I have serious doubts that this is the right moment to introduce EndomorphismRing, just as a container for some true/false questions, when the functionality to do much more is already present -- for instance, computing endomorphism rings numerically isn't limited to genus 2 or to curves defined over QQ (but of course, for analytic methds, the field must be embedded in CC)

comment:21 in reply to: ↑ 20 Changed 12 months ago by edgarcosta

Replying to nbruin:

It sounds like, for now, it should perhaps provide a method endomorphism_ring_is_ZZ on genus 2 curves or their jacobians to provide access. If you're not actually implementing endomorphism rings, you shouldn't be forcing design decisions on them: you'll just make it difficult for the next person who might.

Looking at the code, it look like it has some further functionality, but as far as I can see, these are True/False? checks. I think it's false advertising to hang these things under an "endomorphism_ring" attribute. They are much better as true/false checks on the original object.

Thank you, Nils! I agree with your suggestion! I would suggest attaching the method to the Jacobian.

comment:22 Changed 12 months ago by git

  • Commit changed from e5fb6e0cac6bef8801a84ef27f5de81f05971a8b to d3528482302bf08e15cda1591c83d943386a43fb

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

d352848address review comments, remove EndomorphismRing class

comment:23 Changed 12 months ago by git

  • Commit changed from d3528482302bf08e15cda1591c83d943386a43fb to 14b15c77a4a3eb82b70ff37ecde8ffb7a00b42c5

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

14b15c7Add more apostrophes

comment:24 follow-up: Changed 12 months ago by gh-BarinderBanwait

Dear Edgar, Nils, Travis,

Thank you all very much for your comments and code review. The latest commit has addressed all of the above comments now. Here are the main changes:

  1. The EndomorphismRing class is gone; its two main methods have been attached directly to the Jacobian via is_generic (which existed previously) and geometric_endomorphism_algebra_is_field. I didn't rename is_generic to what Nils suggested - endomorphism_ring_is_ZZ - since I understand that the word "generic" is commonly used to refer to there being geometrically no extra endomorphisms; but let me know if I should change either of these user-facing names.
  1. The file jacobian_endomorphisms.py has been renamed jacobian_endomorphism_utils.py, since it isn't offering any proper implementation of EndomorphismRing, and instead contains the main functions for the two methods in (1) above.
  1. Requisite doc changes throughout.

comment:25 in reply to: ↑ 24 Changed 12 months ago by nbruin

Replying to gh-BarinderBanwait:

  1. The EndomorphismRing class is gone; its two main methods have been attached directly to the Jacobian via is_generic (which existed previously) and geometric_endomorphism_algebra_is_field. I didn't rename is_generic to what Nils suggested - endomorphism_ring_is_ZZ - since I understand that the word "generic" is commonly used to refer to there being geometrically no extra endomorphisms; but let me know if I should change either of these user-facing names.

I have a problem with it, especially when it's attached to the Jacobian itself: it should mention it is measuring something about the endomorphism ring: people could think that it tests whether the torsion is generic?

There is also the slightly pedantic point that any particular jacobian is *not* generic, in the sense that its corresponding point in A_g is closed, and therefore not a generic point.

comment:26 Changed 12 months ago by git

  • Commit changed from 14b15c77a4a3eb82b70ff37ecde8ffb7a00b42c5 to 817bdc57324939c531453bf7987c83082ef0579a

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

817bdc5rename is_generic to geometric_endomorphism_ring_is_ZZ

comment:27 Changed 12 months ago by gh-BarinderBanwait

Thanks Nils, what you say makes sense -- I've renamed it to geometric_endomorphism_ring_is_ZZ

comment:28 Changed 11 months ago by git

  • Commit changed from 817bdc57324939c531453bf7987c83082ef0579a to 462db22ebabae2134de4a3d4f156bb68b698e2d9

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

462db22hopefully fix tests

comment:29 Changed 11 months ago by gh-BarinderBanwait

Hi @edgarcosta, it looks like the tests have passed on this. (It reports "PluginFailed?" on the patchbot site in startup_modules, but clicking through into that it reports that "All tests passed!" so I don't know what's going on there). Is this now ready for the next stage of "positive review", and if so, what are the next steps to get this into the next release?

comment:30 Changed 11 months ago by edgarcosta

  • Cc roed nbruin added
  • Reviewers set to Edgar Costa, Nils
  • Status changed from needs_review to positive_review

I have marked it with a positive review. I believe don't need to do anything else, but I CCed David as he knows more than me.

@nbruin, I also added you the list of Reviewers, if you disagree with the review or that please edit accordingly.

comment:31 Changed 11 months ago by edgarcosta

  • Reviewers changed from Edgar Costa, Nils to Edgar Costa, Nils Bruin

comment:32 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:33 Changed 10 months ago by vbraun

  • Branch changed from u/gh-BarinderBanwait/absolutely_simple_endomorphism_rings to 462db22ebabae2134de4a3d4f156bb68b698e2d9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.