sage.geometry.polyhedron.library: Delay import of rings

[mkoeppe]

cherry-picked from #32432 (sagemath-polyhedra)

CC: @kliem

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 59c0973

Reviewer: Jonathan Kliem

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

[mkoeppe]

Branch: u/mkoeppe/sage_geometry_polyhedron_library__delay_import_of_rings

[mkoeppe]

New commits:

1305b3a	src/sage/geometry/polyhedron/library.py: Make imports from sage.rings lazier
ae64d1b	src/sage/geometry/polyhedron/library.py: Make some imports lazy
459b3d4	src/sage/geometry/polyhedron/library.py: Ignore failing imports from other modules
24e821d	src/sage/geometry/polyhedron/library.py: Move import of RDF into methods

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

Commit: 24e821d

[sagetrac-git]

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

853cfb7

src/sage/geometry/polyhedron/library.py: Add missing import of RDF

[sagetrac-git]

Changed commit from 24e821d to 853cfb7

[kliem]

comment:6

I don't understand:

+                from sage.rings.qqbar import AA as base_ring

Why do we do a local import instead of resolving the global lazy import?

[kliem]

comment:7

Otherwise this looks good to me.
Is this ticket missing from #32432?

[sagetrac-git]

Changed commit from 853cfb7 to 0902757

[sagetrac-git]

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

0902757

src/sage/geometry/polyhedron/library.py: Simplify use of AA

[kliem]

comment:9

Green bot => Positive review.

[kliem]

Reviewer: Jonathan Kliem

[mkoeppe]

comment:10

Replying to @kliem:

Is this ticket missing from #32432?

I've added it

[mkoeppe]

comment:11

various failures like this:

sage -t --long --random-seed=272753705677125902426358705129228538309 src/sage/geometry/voronoi_diagram.py
**********************************************************************
File "src/sage/geometry/voronoi_diagram.py", line 55, in sage.geometry.voronoi_diagram.VoronoiDiagram
Failed example:
    DV = VoronoiDiagram([[AA(c) for c in v] for v in polytopes.regular_polygon(5).vertices_list()]); DV
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.voronoi_diagram.VoronoiDiagram[2]>", line 1, in <module>
        DV = VoronoiDiagram([[AA(c) for c in v] for v in polytopes.regular_polygon(Integer(5)).vertices_list()]); DV
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/library.py", line 578, in regular_polygon
        return Polyhedron(vertices=verts, base_ring=base_ring, backend=backend)
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/constructor.py", line 674, in Polyhedron
        parent = Polyhedra(base_ring, ambient_dim, backend=backend)
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/parent.py", line 147, in Polyhedra
        raise ValueError("invalid base ring: {} cannot be coerced to a real field".format(base_ring))
    ValueError: invalid base ring: Algebraic Real Field cannot be coerced to a real field
**********************************************************************

[kliem]

comment:12

Looks like a bug with lazy import and coercion:

sage: cm = coercion_model
sage: cm.analyse(RR, AA)
(['Coercion on right operand via',
  Conversion via _mpfr_ method map:
    From: Algebraic Real Field
    To:   Real Field with 53 bits of precision,
  'Arithmetic performed after coercions.'],
 Real Field with 53 bits of precision)
sage: lazy_import('sage.rings.qqbar', ['AA', 'QQbar'])
sage: cm.analyse(RR, AA)
(['Right operand is not Sage element, will try _sage_.'], None)

[kliem]

comment:13

I see four ways to deal with it:

• Resolve LazyImport somewhere in the coercion model.
• Add a custom __instancecheck__ in Parent.
• Fix parent(foo), when foo is a LazyImport instance.
• Accept this shortcoming of lazy imports and just revert the last commit (and comment that coercion with lazy imports does not work currently).

What do you think of it?

[mkoeppe]

comment:14

Thanks for investigating this! I've opened #32806 for this.

For the present ticket, I'll just revert the last commit and perhaps also get rid of use of lazy_import for the other imported parents in this file, for uniformity.

[sagetrac-git]

Changed commit from 0902757 to 59c0973

[sagetrac-git]

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

e5aa986	src/sage/geometry/polyhedron/library.py: Replace lazy_import of AA, QQbar by method-local imports
59c0973	Merge tag '9.5.beta5' into t/32780/sage_geometry_polyhedron_library__delay_import_of_rings

[mkoeppe]

comment:17

The remaining failures:

sage -t --long --random-seed=229537426550331149451412980195837018046 src/sage/modular/modform/numerical.py  # 3 doctests failed
sage -t --long --random-seed=229537426550331149451412980195837018046 src/sage/dynamics/arithmetic_dynamics/projective_ds.py  # 1 doctest failed

are unrelated

[kliem]

comment:18

LGTM.

[mkoeppe]

comment:19

Thanks!

[vbraun]

Changed branch from u/mkoeppe/sage_geometry_polyhedron_library__delay_import_of_rings to 59c0973