Opened 2 years ago

Closed 2 years ago

#23175 closed enhancement (fixed)

Implement computation of Riemann period matrices etc.

Reported by: nbruin Owned by:
Priority: major Milestone: sage-8.0
Component: algebraic geometry Keywords: sd86.5
Cc: aly.deines Merged in:
Authors: Nils Bruin, Alexandre Zotine Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 6e689c2 (Commits) Commit: 6e689c2aa87ec16fa6ce335e171aa4bce1885120
Dependencies: #23140 Stopgaps:

Description

Include a class that supports analytic computation of period matrices and computation of endomorphism matrices.

Change History (32)

comment:1 Changed 2 years ago by nbruin

  • Branch set to u/nbruin/riemann_surface

comment:2 Changed 2 years ago by nbruin

  • Commit set to 3ffce6a66ed253e5592f88010cb98beddeca794c
  • Dependencies set to #23140
  • Keywords sd86.5 added

We'll likely be rewriting history on this branch.


New commits:

f128723Creation of a vectorized gauss_legendre integrator
8c82320Change to node for degree=1, based on bugfix in mpmath:
a455f3esome PEP8, reviewer comments; rationalized "degree" parameter for "nodes"
3ffce6aInitial check-in of RiemannSurface

comment:3 Changed 2 years ago by git

  • Commit changed from 3ffce6a66ed253e5592f88010cb98beddeca794c to ce9fca4f7042629ad0e733e9bf87044cfa668597

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

ce9fca4some further imports

comment:4 Changed 2 years ago by git

  • Commit changed from ce9fca4f7042629ad0e733e9bf87044cfa668597 to 0143571215b3720f77a4f62db5d1e9e235758d08

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

0143571renamed file

comment:5 Changed 2 years ago by git

  • Commit changed from 0143571215b3720f77a4f62db5d1e9e235758d08 to c9edca9a447d5f9fcfa42f939c70cbaa59e126e1

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

b543141Minor updates to documentation.
b21c02aAdded a few imports to documentation.
c9edca9small changes

comment:6 Changed 2 years ago by git

  • Commit changed from c9edca9a447d5f9fcfa42f939c70cbaa59e126e1 to c26f623ce8e5cf57cf8402d84a139220f75ea644

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

c26f623avoid integer division in python code

comment:7 Changed 2 years ago by git

  • Commit changed from c26f623ce8e5cf57cf8402d84a139220f75ea644 to 8f8dbb1d48834aa301bc8f6bd82b7c122ffcf890

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

8f8dbb1avoid integer division in python code

comment:8 Changed 2 years ago by git

  • Commit changed from 8f8dbb1d48834aa301bc8f6bd82b7c122ffcf890 to 54bdd617f2bf7c6f177123bbdc7adffd5673854f

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

bec1bf3Fixed a doctest.
54bdd61Merge branch 'u/nbruin/riemann_surface' of git://trac.sagemath.org/sage into t/23175/riemann_surface

comment:9 Changed 2 years ago by git

  • Commit changed from 54bdd617f2bf7c6f177123bbdc7adffd5673854f to 5b1c2d61c9f88c175b8edb15ad69ca2307163ae3

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

5b1c2d6Initial Check-in of RiemannSurface

comment:10 Changed 2 years ago by nbruin

  • Cc aly.deines added
  • Status changed from new to needs_review

Documentation should now be more or less up to standards.

comment:11 follow-up: Changed 2 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

I am making some style changes. I am still working on this, but here are already some comments.

Please fix/comment the following:

  • I guess you want to make this more accessible than from sage.schemes.riemann_surfaces.riemann_surface import RiemannSurface?
  • Why so much precision in the examples? Having 200 bits of precision seems like a lot. I tried a couple of the examples with less precision and found the output much easier to parse.
  • TODO: have an example where self.genus < 0 to raise error
  • Comment on the constant 100 in _determine_new_w and _newton_iteration (nothing fancy necessary, just say that this is a random bound that worked well in practice?)
  • In period_matrix: Computes the period matrix of f given the homology basis and cohomology basis. What is "given" here?
  • Why do you perform your own caching in period_matrix (self._PM) and riemann_matrix (self._RM), cohomology_basis (self._differentials) ?
  • The self._differentials[1] is very confusing. Why do you need it?
  • raise ValueError('Length of differentials list is not equal to genus.') should this not be a NotImplementedError?
  • Does simple_vector_line_integral require a previous call to cohomology_basis to work? (The docstring makes it seem so.) If this is the case, then the method should probably not be public. In other words, the NOTE there is confusing.
  • The if/else in homology_basis…could you add a comment on why you can be sure that cds[0] != cds[1]? If this could go wrong in low precision, you might want to replace the assert with something else.
  • Should RiemannSurface really be an object and not at least a SageObject? You could probably add a comment about why this is not a scheme in the hierarchy of the namespace there.

There are some confusing comments in the source code, please try to make them more understandable to people who did not write this code:

# Using singular to compute the genus quickly. Later, use rank/2 of
        # gram matrix?

        # TODO: can we get rid of the comments below?
        # I'm not sure if this check will work, but could be useful?
        # I think singular's genus computation returns a negative genus
        # if it's reducible. Either way, if the genus is negative we should
        # terminate.

#TODO: Why not make a list in the first place with the edges?

        #mt = Matrix(ZZ,[[1 if i==j else 0 for j in range(4*g**2)] +
        #  [((S*w.monomial_coefficient(vars[i]).real_part()).round() for w in W] +
        #  [S*w.monomial_coefficient(vars[i]).imag_part()).round() for w in W] for i in range(len(vars))])
Last edited 2 years ago by saraedum (previous) (diff)

comment:12 Changed 2 years ago by saraedum

  • Branch changed from u/nbruin/riemann_surface to u/saraedum/riemann_surface

comment:13 Changed 2 years ago by saraedum

  • Commit changed from 5b1c2d61c9f88c175b8edb15ad69ca2307163ae3 to 4b3eeea12060e9d2dc38ab4c1ee70ae8c2cacd45

I hope you do not mind my changes. And sorry for writing so many comments. You don't really have to fix all of them, it's just things I stumbled upon so I thought I could as well mention them.


New commits:

415e21dMerge branch 'develop' into t/23175/riemann_surface
4b3eeeaSome reviewer style changes

comment:14 in reply to: ↑ 11 Changed 2 years ago by nbruin

Replying to saraedum:

I am making some style changes.

Thank you for style formatting!

  • I guess you want to make this more accessible than from sage.schemes.riemann_surfaces.riemann_surface import RiemannSurface?

Probably affine and projective plane curves will eventually grow a method "riemann_surface", so I don't think accessibility is an issue. RiemannSurface can mean more than just a compact one, so I'd be careful exposing it in the global namespace for that reason under that name (and other names get unappetizing pretty quickly)

  • Why so much precision in the examples? Having 200 bits of precision seems like a lot. I tried a couple of the examples with less precision and found the output much easier to parse.

Turned it back a little

  • TODO: have an example where self.genus < 0 to raise error

Removed it. Actually, surfaces with multiple components shouldn't be an issue. The singular backend doesn't like them, though.

  • Comment on the constant 100 in _determine_new_w and _newton_iteration (nothing fancy necessary, just say that this is a random bound that worked well in practice?)

Done. This will never be triggered in practice.

  • In period_matrix: Computes the period matrix of f given the homology basis and cohomology basis. What is "given" here?

Fixed

  • Why do you perform your own caching in period_matrix (self._PM) and riemann_matrix (self._RM),

We did this to accommodate for the different options in the cohomology basis. However, in practice nobody would change that, and in actuality there's a benefit in providing the user a hook to specify this him/herself. This is implemented now, so the period matrix can now be cached in a vanilla way (and the riemann matrix is easily derived, so doesn't need to be cached)

cohomology_basis (self._differentials) ?

That's because the attribute can be specified in multiple ways.

  • The self._differentials[1] is very confusing. Why do you need it?

Removed now, in favour of a user-configurable "differentials" value.

  • raise ValueError('Length of differentials list is not equal to genus.') should this not be a NotImplementedError?

No, that's an error. "Genus" here should be H0(k,Omega1_S). Thus the genus of the union of two elliptic curves should be 2.

  • Does simple_vector_line_integral require a previous call to cohomology_basis to work? (The docstring makes it seem so.) If this is the case, then the method should probably not be public. In other words, the NOTE there is confusing.

No, it's a useful routine in general, but it does need stuff preinitialized (you'll get an error otherwise, no incorrect results). It's pretty inner-loop otherwise, so it shouldn't be weighed down with an expensive call to homology_basis.

  • The if/else in homology_basis…could you add a comment on why you can be sure that cds[0] != cds[1]? If this could go wrong in low precision, you might want to replace the assert with something else.

There are much worse ways in which that code can fail than the assert. I don't think it's worth worrying about.

  • Should RiemannSurface really be an object and not at least a SageObject?

I don't see a benefit from making it a SageObject. I don't see how this object would benefit from interacting more closely with sage objects. It's just a container to spit out Riemann matrices.

You could probably add a comment about why this is not a scheme in the hierarchy of the namespace there.

I don't think I have a comment on that.

There are some confusing comments in the source code, please try to make them more understandable to people who did not write this code:

# Using singular to compute the genus quickly. Later, use rank/2 of
        # gram matrix?

Gone

        # TODO: can we get rid of the comments below?
        # I'm not sure if this check will work, but could be useful?
        # I think singular's genus computation returns a negative genus
        # if it's reducible. Either way, if the genus is negative we should
        # terminate.

fixed

#TODO: Why not make a list in the first place with the edges?

fixed

        #mt = Matrix(ZZ,[[1 if i==j else 0 for j in range(4*g**2)] +
        #  [((S*w.monomial_coefficient(vars[i]).real_part()).round() for w in W] +
        #  [S*w.monomial_coefficient(vars[i]).imag_part()).round() for w in W] for i in range(len(vars))])

removed

comment:15 Changed 2 years ago by nbruin

  • Branch changed from u/saraedum/riemann_surface to u/nbruin/riemann_surface

comment:16 Changed 2 years ago by nbruin

  • Commit changed from 4b3eeea12060e9d2dc38ab4c1ee70ae8c2cacd45 to 358f995077d983e1efedfe4de3950973910fea42
  • Status changed from needs_work to needs_review

New commits:

358f995Bugfixes and review suggestions.

comment:17 Changed 2 years ago by git

  • Commit changed from 358f995077d983e1efedfe4de3950973910fea42 to f8b255b2abb1d6091dd783db35232d5b5ec1083a

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

f8b255bBugfixes and review suggestions.

comment:18 Changed 2 years ago by saraedum

Ok. Feel free to set this to positive review when the patchbot is happy.

comment:19 Changed 2 years ago by git

  • Commit changed from f8b255b2abb1d6091dd783db35232d5b5ec1083a to 34ac9e1ee7523a9e926fa1582f4d1ace7238e939

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

34ac9e1Several fixes:

comment:20 Changed 2 years ago by nbruin

Sorry! I ran into one case where the Voronoi cells as computed did not give rise to a homology basis (because we were discarding edges in a way that led to an non-connected diagram). I rewrote another routine to be better documented and clearer as well.

Knock-on effect of the changed voronoi cells is that a lot of doctests with arbitrary output changed, but those are not very insightful. Nonetheless, probably good form if the reviewer gives a thumbs-up to these changes before we set this to positive.

comment:21 Changed 2 years ago by git

  • Commit changed from 34ac9e1ee7523a9e926fa1582f4d1ace7238e939 to ce5097a4dcb6bf45a11078b3a91fc926c2b73592

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

ce5097aTwo more fixes:

comment:22 Changed 2 years ago by git

  • Commit changed from ce5097a4dcb6bf45a11078b3a91fc926c2b73592 to 32856842d33c8b378d354331ddc12dfd7f5884bc

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

3285684Use sage_mantissa_exponent to get a cheaper log2

comment:23 Changed 2 years ago by git

  • Commit changed from 32856842d33c8b378d354331ddc12dfd7f5884bc to 4eaaff07e7c64df6f205d9b9293458d1d755b1a9

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

4eaaff0correction to implementation of voronoi_ghost

comment:24 Changed 2 years ago by saraedum

Feel free to set this back to positive review if you're confident that tests are going to pass.

comment:25 Changed 2 years ago by nbruin

OK. the bots seem busy. I've tested on 8.0beta11 and all tests pass. A previous complaint by a plugin about "EXAMPLE:" rather than "EXAMPLES:" has been fixed too, so following Julian's assessment: positive review. Preferably new issues get their own ticket.

comment:26 Changed 2 years ago by nbruin

  • Status changed from needs_review to positive_review

comment:27 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails on 32-bit:

sage -t --long src/sage/schemes/riemann_surfaces/riemann_surface.py
**********************************************************************
File "src/sage/schemes/riemann_surfaces/riemann_surface.py", line 806, in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.upstairs_edges
Failed example:
    edgeset = S.upstairs_edges(); edgeset
Expected:
    [[(0, 0), (1, 0)],
     [(0, 1), (1, 1)],
     [(0, 0), (5, 1)],
     [(0, 1), (5, 0)],
     [(1, 0), (4, 1)],
     [(1, 1), (4, 0)],
     [(2, 0), (3, 1)],
     [(2, 1), (3, 0)],
     [(2, 0), (4, 0)],
     [(2, 1), (4, 1)],
     [(3, 0), (5, 0)],
     [(3, 1), (5, 1)],
     [(4, 0), (5, 0)],
     [(4, 1), (5, 1)]]
Got:
    [[(0, 0), (1, 1)],
     [(0, 1), (1, 0)],
     [(0, 0), (5, 0)],
     [(0, 1), (5, 1)],
     [(1, 0), (4, 0)],
     [(1, 1), (4, 1)],
     [(2, 0), (3, 0)],
     [(2, 1), (3, 1)],
     [(2, 0), (4, 1)],
     [(2, 1), (4, 0)],
     [(3, 0), (5, 1)],
     [(3, 1), (5, 0)],
     [(4, 0), (5, 0)],
     [(4, 1), (5, 1)]]
**********************************************************************
File "src/sage/schemes/riemann_surfaces/riemann_surface.py", line 1423, in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.period_matrix
Failed example:
    S.period_matrix() # abs tol 0.000001
Expected:
    [-0.77519780590366792 + 0.61819962132820207*I  -0.19145804690473785 - 1.3890819401503317*I  -2.1720559850746018 + 0.49575760460334677*I -0.23874427945779023 - 0.49575760460334677*I -0.43020232636252808 - 0.89332433554698493*I  -0.96665585280840577 - 2.0072815614785338*I]
    [  2.1720559850746018 + 0.49575760460334677*I  -3.1387118378830076 - 0.27512471421878286*I  -0.53645352644587770 - 1.1139572259315488*I    1.3968581791709339 + 1.1139572259315488*I   -1.7418536587120737 - 1.3890819401503317*I -0.96665585280840577 - 0.77088231882212961*I]
    [  0.53645352644587769 - 1.1139572259315488*I  -1.5031093792542835 - 0.89332433554698493*I  0.77519780590366793 + 0.61819962132820208*I   2.7085095115204795 - 0.61819962132820208*I   1.2054001322661960 - 0.27512471421878285*I -0.96665585280840577 + 0.22063289038456391*I]
Got:
    [     0.43020232636252809 - 1.3345901163161127*I     0.96665585280840577 - 0.77088231882212962*I      2.1720559850746018 + 0.49575760460334669*I  1.1926223897340549e-18 - 0.99151520920669346*I     0.96665585280840577 - 0.22063289038456391*I  -2.3852447794681098e-18 + 2.2279144518630976*I]
    [      1.7418536587120737 + 2.6254811828067359*I     0.96665585280840577 + 0.22063289038456391*I      0.53645352644587770 - 1.1139572259315489*I  -2.1684043449710089e-19 + 2.2279144518630977*I      0.96665585280840577 + 2.0072815614785338*I  -2.6020852139652106e-18 - 1.2363992426564042*I]
    [     -1.2054001322661960 + 1.2666399234254763*I      0.96665585280840577 - 2.0072815614785338*I    -0.77519780590366792 + 0.61819962132820217*I   3.2526065174565133e-19 - 1.2363992426564042*I     0.96665585280840577 + 0.77088231882212962*I -4.3368086899420177e-18 - 0.99151520920669343*I]
Tolerance exceeded in 33 of 36:
    -0.77519780590366792 vs 0.43020232636252809, tolerance 1e+00 > 1e-06
    + 0.61819962132820207 vs - 1.3345901163161127, tolerance 2e+00 > 1e-06
    -0.19145804690473785 vs 0.96665585280840577, tolerance 1e+00 > 1e-06
    - 1.3890819401503317 vs - 0.77088231882212962, tolerance 6e-01 > 1e-06
    -2.1720559850746018 vs 2.1720559850746018, tolerance 4e+00 > 1e-06
    -0.23874427945779023 vs 1.1926223897340549e-18, tolerance 2e-01 > 1e-06
    - 0.49575760460334677 vs - 0.99151520920669346, tolerance 5e-01 > 1e-06
    -0.43020232636252808 vs 0.96665585280840577, tolerance 1e+00 > 1e-06
    - 0.89332433554698493 vs - 0.22063289038456391, tolerance 7e-01 > 1e-06
    -0.96665585280840577 vs -2.3852447794681098e-18, tolerance 1e+00 > 1e-06
    - 2.0072815614785338 vs + 2.2279144518630976, tolerance 4e+00 > 1e-06
    2.1720559850746018 vs 1.7418536587120737, tolerance 4e-01 > 1e-06
    + 0.49575760460334677 vs + 2.6254811828067359, tolerance 2e+00 > 1e-06
    -3.1387118378830076 vs 0.96665585280840577, tolerance 4e+00 > 1e-06
    - 0.27512471421878286 vs + 0.22063289038456391, tolerance 5e-01 > 1e-06
    -0.53645352644587770 vs 0.53645352644587770, tolerance 1e+00 > 1e-06
    1.3968581791709339 vs -2.1684043449710089e-19, tolerance 1e+00 > 1e-06
    + 1.1139572259315488 vs + 2.2279144518630977, tolerance 1e+00 > 1e-06
    -1.7418536587120737 vs 0.96665585280840577, tolerance 3e+00 > 1e-06
    - 1.3890819401503317 vs + 2.0072815614785338, tolerance 3e+00 > 1e-06
    -0.96665585280840577 vs -2.6020852139652106e-18, tolerance 1e+00 > 1e-06
    - 0.77088231882212961 vs - 1.2363992426564042, tolerance 5e-01 > 1e-06
    0.53645352644587769 vs -1.2054001322661960, tolerance 2e+00 > 1e-06
    - 1.1139572259315488 vs + 1.2666399234254763, tolerance 2e+00 > 1e-06
    -1.5031093792542835 vs 0.96665585280840577, tolerance 2e+00 > 1e-06
    - 0.89332433554698493 vs - 2.0072815614785338, tolerance 1e+00 > 1e-06
    0.77519780590366793 vs -0.77519780590366792, tolerance 2e+00 > 1e-06
    2.7085095115204795 vs 3.2526065174565133e-19, tolerance 3e+00 > 1e-06
    - 0.61819962132820208 vs - 1.2363992426564042, tolerance 6e-01 > 1e-06
    1.2054001322661960 vs 0.96665585280840577, tolerance 2e-01 > 1e-06
    - 0.27512471421878285 vs + 0.77088231882212962, tolerance 1e+00 > 1e-06
    -0.96665585280840577 vs -4.3368086899420177e-18, tolerance 1e+00 > 1e-06
    + 0.22063289038456391 vs - 0.99151520920669343, tolerance 1e+00 > 1e-06
**********************************************************************
File "src/sage/schemes/riemann_surfaces/riemann_surface.py", line 1454, in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.riemann_matrix
Failed example:
    S.riemann_matrix() #abs tol 0.0000001
Expected:
    [        0.50000000000000000 + 1.3228756555322953*I       -0.25000000000000000 + 0.66143782776614765*I  6.7220534694101275e-18 - 2.1684043449710089e-18*I]
    [      -0.25000000000000000 + 0.66143782776614764*I        0.25000000000000000 + 0.66143782776614765*I     0.50000000000000000 - 1.7347234759768071e-18*I]
    [-6.5052130349130266e-19 + 3.4694469519536142e-18*I     0.50000000000000000 + 3.4694469519536142e-18*I        0.25000000000000000 + 0.66143782776614765*I]
Got:
    [       0.37500000000000000 + 0.33071891388307383*I     0.50000000000000000 - 6.9388939039072284e-18*I    -0.50000000000000001 - 7.9146758591441824e-18*I]
    [    0.50000000000000001 - 4.3368086899420177e-18*I        0.24999999999999999 + 0.66143782776614764*I -1.1275702593849246e-17 + 1.3877787807814457e-17*I]
    [   -0.50000000000000000 + 1.5178830414797062e-17*I  2.7105054312137611e-18 - 5.4210108624275222e-18*I        0.24999999999999998 + 0.66143782776614762*I]
Tolerance exceeded in 10 of 18:
    0.50000000000000000 vs 0.37500000000000000, tolerance 1e-01 > 1e-07
    + 1.3228756555322953 vs + 0.33071891388307383, tolerance 1e+00 > 1e-07
    -0.25000000000000000 vs 0.50000000000000000, tolerance 8e-01 > 1e-07
    + 0.66143782776614765 vs - 6.9388939039072284e-18, tolerance 7e-01 > 1e-07
    6.7220534694101275e-18 vs -0.50000000000000001, tolerance 5e-01 > 1e-07
    -0.25000000000000000 vs 0.50000000000000001, tolerance 8e-01 > 1e-07
    + 0.66143782776614764 vs - 4.3368086899420177e-18, tolerance 7e-01 > 1e-07
    0.50000000000000000 vs -1.1275702593849246e-17, tolerance 5e-01 > 1e-07
    -6.5052130349130266e-19 vs -0.50000000000000000, tolerance 5e-01 > 1e-07
    0.50000000000000000 vs 2.7105054312137611e-18, tolerance 5e-01 > 1e-07
**********************************************************************
3 items had failures:
   1 of   6 in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.period_matrix
   1 of   6 in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.riemann_matrix
   1 of   6 in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurface.upstairs_edges
    [206 tests, 3 failures, 28.84 s]

comment:28 Changed 2 years ago by git

  • Commit changed from 4eaaff07e7c64df6f205d9b9293458d1d755b1a9 to 6e689c2aa87ec16fa6ce335e171aa4bce1885120

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

6e689c2make doctests more robust

comment:29 Changed 2 years ago by nbruin

  • Status changed from needs_work to needs_review

Sigh ... doctesting doctests with non-uniquely represented objects is a pain. It would be nice if we'd have a way of getting wider architecture exposure without Volker having to intervene. Let's try this.

comment:30 Changed 2 years ago by saraedum

  • Status changed from needs_review to positive_review

comment:31 Changed 2 years ago by saraedum

Looks good. (Yes, I guess a better CI/CD infrastructure would be helpful. As I am just transferring the company I work for to gitlab CI/CD, I discussed this quite a bit with roed during the past few days actually. But I guess it would be quite some effort to swap our patchbot/buildbot out for a more standardized solution…)

comment:32 Changed 2 years ago by vbraun

  • Branch changed from u/nbruin/riemann_surface to 6e689c2aa87ec16fa6ce335e171aa4bce1885120
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.