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

Implement computation of Riemann period matrices etc. #23175

Closed
nbruin opened this issue Jun 8, 2017 · 49 comments
Closed

Implement computation of Riemann period matrices etc. #23175

nbruin opened this issue Jun 8, 2017 · 49 comments

Comments

@nbruin
Copy link
Contributor

nbruin commented Jun 8, 2017

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

Depends on #23140

CC: @adeines

Component: algebraic geometry

Keywords: sd86.5

Author: Nils Bruin, Alexandre Zotine

Branch/Commit: 6e689c2

Reviewer: Julian Rüth

Issue created by migration from https://trac.sagemath.org/ticket/23175

@nbruin nbruin added this to the sage-8.0 milestone Jun 8, 2017
@nbruin
Copy link
Contributor Author

nbruin commented Jun 8, 2017

Branch: u/nbruin/riemann_surface

@nbruin
Copy link
Contributor Author

nbruin commented Jun 8, 2017

comment:2

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

@nbruin
Copy link
Contributor Author

nbruin commented Jun 8, 2017

Dependencies: #23140

@nbruin
Copy link
Contributor Author

nbruin commented Jun 8, 2017

Changed keywords from none to sd86.5

@nbruin
Copy link
Contributor Author

nbruin commented Jun 8, 2017

Commit: 3ffce6a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2017

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

ce9fca4some further imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2017

Changed commit from 3ffce6a to ce9fca4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2017

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

0143571renamed file

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 8, 2017

Changed commit from ce9fca4 to 0143571

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

Changed commit from 0143571 to c9edca9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

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

c26f623avoid integer division in python code

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

Changed commit from c9edca9 to c26f623

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

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

8f8dbb1avoid integer division in python code

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

Changed commit from c26f623 to 8f8dbb1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

Changed commit from 8f8dbb1 to 54bdd61

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2017

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

5b1c2d6Initial Check-in of RiemannSurface

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2017

Changed commit from 54bdd61 to 5b1c2d6

@nbruin
Copy link
Contributor Author

nbruin commented Jun 11, 2017

comment:10

Documentation should now be more or less up to standards.

@saraedum
Copy link
Member

Reviewer: Julian Rüth

@saraedum
Copy link
Member

comment:11

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

@saraedum
Copy link
Member

Changed branch from u/nbruin/riemann_surface to u/saraedum/riemann_surface

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

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

f8b255bBugfixes and review suggestions.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

Changed commit from 358f995 to f8b255b

@saraedum
Copy link
Member

comment:18

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

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

34ac9e1Several fixes:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

Changed commit from f8b255b to 34ac9e1

@nbruin
Copy link
Contributor Author

nbruin commented Jun 14, 2017

comment:20

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

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

ce5097aTwo more fixes:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

Changed commit from 34ac9e1 to ce5097a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

3285684Use sage_mantissa_exponent to get a cheaper log2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from ce5097a to 3285684

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

Changed commit from 3285684 to 4eaaff0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2017

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

4eaaff0correction to implementation of voronoi_ghost

@saraedum
Copy link
Member

comment:24

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

@nbruin
Copy link
Contributor Author

nbruin commented Jun 16, 2017

comment:25

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.

@vbraun
Copy link
Member

vbraun commented Jun 17, 2017

comment:27

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]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2017

Changed commit from 4eaaff0 to 6e689c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2017

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

6e689c2make doctests more robust

@nbruin
Copy link
Contributor Author

nbruin commented Jun 17, 2017

comment:29

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.

@saraedum
Copy link
Member

comment:31

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

@vbraun
Copy link
Member

vbraun commented Jun 22, 2017

Changed branch from u/nbruin/riemann_surface to 6e689c2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants