sagemath/sage

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

Closed this issue · 24 comments

mkoeppe commented

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 commented

New commits:

1305b3asrc/sage/geometry/polyhedron/library.py: Make imports from sage.rings lazier
ae64d1bsrc/sage/geometry/polyhedron/library.py: Make some imports lazy
459b3d4src/sage/geometry/polyhedron/library.py: Ignore failing imports from other modules
24e821dsrc/sage/geometry/polyhedron/library.py: Move import of RDF into methods
mkoeppe commented

Author: Matthias Koeppe

mkoeppe commented

Commit: 24e821d

mkoeppe commented

Description changed:

--- 
+++ 
@@ -1,2 +1,2 @@
-for #32432 (**sagemath-polyhedra**)
+cherry-picked from #32432 (**sagemath-polyhedra**)
sagetrac-git commented

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

853cfb7src/sage/geometry/polyhedron/library.py: Add missing import of RDF
sagetrac-git commented

Changed commit from 24e821d to 853cfb7

kliem commented
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 commented
comment:7

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

sagetrac-git commented

Changed commit from 853cfb7 to 0902757

sagetrac-git commented

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

0902757src/sage/geometry/polyhedron/library.py: Simplify use of AA
kliem commented
comment:9

Green bot => Positive review.

kliem commented

Reviewer: Jonathan Kliem

mkoeppe commented
comment:10

Replying to @kliem:

Is this ticket missing from #32432?

I've added it

mkoeppe commented
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 commented
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 commented
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 commented
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 commented

Changed commit from 0902757 to 59c0973

sagetrac-git commented

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

e5aa986src/sage/geometry/polyhedron/library.py: Replace lazy_import of AA, QQbar by method-local imports
59c0973Merge tag '9.5.beta5' into t/32780/sage_geometry_polyhedron_library__delay_import_of_rings
mkoeppe commented
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 commented
comment:18

LGTM.

mkoeppe commented
comment:19

Thanks!