sagemath/sage

Move graphs methods to Polyhedron_base4

Closed this issue · 16 comments

kliem commented

Part of #32651.

We also move neighborliness to Polyhedron_base3 (missed on #32884)
and _sage_input_ and _delete to Polyhedron_base0.

Depends on #32884

CC: @mkoeppe

Component: geometry

Author: Jonathan Kliem

Branch/Commit: ff8c7ef

Reviewer: Matthias Koeppe

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

comment:2

Pyflakes plugin patchbot says:

src/sage/geometry/polyhedron/base.py:1378:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base.py:3876:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base3.py:1941:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base4.py:1186:13 'sage.graphs.graph' imported but unused

I just also mentionned it in
#32174 comment:41
but I guess it is better to fix that here to avoid later conflicts.

comment:3

There is a failing doctest on the patchbot, but maybe it is unrelated:

sage -t --long --random-seed=257293673212457698646690051018428858654 src/sage/geometry/hyperbolic_space/hyperbolic_model.py
**********************************************************************
File "src/sage/geometry/hyperbolic_space/hyperbolic_model.py", line 567, in sage.geometry.hyperbolic_space.hyperbolic_model.HyperbolicModel.random_geodesic
Failed example:
    bool((h.endpoints()[0].coordinates()).imag() >= 0)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of   3 in sage.geometry.hyperbolic_space.hyperbolic_model.HyperbolicModel.random_geodesic
    [234 tests, 1 failure, 1.55 s]
kliem commented
comment:4

Replying to @seblabbe:

Pyflakes plugin patchbot says:

src/sage/geometry/polyhedron/base.py:1378:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base.py:3876:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base3.py:1941:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base4.py:1186:13 'sage.graphs.graph' imported but unused

I just also mentionned it in

#32174 comment:41
but I guess it is better to fix that here to avoid later conflicts.

Nothing new, just moved that code. It's all in test methods, that do not test in case features relying on graphs. E.g.

    def _test_combinatorial_face_as_combinatorial_polyhedron(self, tester=None, **options):
    ...
        try:
            import sage.graphs.graph
        except:
            pass
        else:
            tester.assertTrue(P.combinatorial_polyhedron().vertex_facet_graph().is_isomorphic(D1.vertex_facet_graph()))
    ...
kliem commented
comment:5

Replying to @seblabbe:

There is a failing doctest on the patchbot, but maybe it is unrelated:

sage -t --long --random-seed=257293673212457698646690051018428858654 src/sage/geometry/hyperbolic_space/hyperbolic_model.py
**********************************************************************
File "src/sage/geometry/hyperbolic_space/hyperbolic_model.py", line 567, in sage.geometry.hyperbolic_space.hyperbolic_model.HyperbolicModel.random_geodesic
Failed example:
    bool((h.endpoints()[0].coordinates()).imag() >= 0)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of   3 in sage.geometry.hyperbolic_space.hyperbolic_model.HyperbolicModel.random_geodesic
    [234 tests, 1 failure, 1.55 s]

I opened #32891.

Reviewer: Matthias Koeppe

kliem commented
comment:7

Replying to @kliem:

Replying to @seblabbe:

Pyflakes plugin patchbot says:

src/sage/geometry/polyhedron/base.py:1378:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base.py:3876:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base3.py:1941:17 'sage.graphs.graph' imported but unused
src/sage/geometry/polyhedron/base4.py:1186:13 'sage.graphs.graph' imported but unused

I just also mentionned it in

#32174 comment:41
but I guess it is better to fix that here to avoid later conflicts.

Nothing new, just moved that code. It's all in test methods, that do not test in case features relying on graphs. E.g.

    def _test_combinatorial_face_as_combinatorial_polyhedron(self, tester=None, **options):
    ...
        try:
            import sage.graphs.graph
        except:
            pass
        else:
            tester.assertTrue(P.combinatorial_polyhedron().vertex_facet_graph().is_isomorphic(D1.vertex_facet_graph()))
    ...

Instead of

try:
    import sage.graphs.graphh
except ImportError:
    pass
else:
    # do something

We could also do something as:

from importlib.util import find_spec
if find_spec("sage.graphs") and find_spec("sage.graphs.graph"):
    # do something
comment:8

Or can we use newly introduced sage features as in:

from sage.features.sagemath import sage__graphs
if sage__graphs().is_present():
   ...

?

comment:9

Looks like this has a merge conflict with 9.5.beta7

kliem commented
comment:10

That is what happend since 9.5.beta6:

git diff develop 6ec717a56dcb0fd629ca850d9b9391ea8d96ccac base.py
diff --git a/src/sage/geometry/polyhedron/base.py b/src/sage/geometry/polyhedron/base.py
index c1d27faebd..23e6889087 100644
--- a/src/sage/geometry/polyhedron/base.py
+++ b/src/sage/geometry/polyhedron/base.py
@@ -7225,7 +7225,7 @@ class Polyhedron_base(Polyhedron_base1):
             sage: Q.volume.is_in_cache()
             True
         """
-        from sage.features import FeatureNotPresentError
+        from sage.features import FeatureNotPresentError, PythonModule
         if measure == 'induced_rational' and engine not in ['auto', 'latte', 'normaliz']:
             raise RuntimeError("the induced rational measure can only be computed with the engine set to `auto`, `latte`, or `normaliz`")
         if measure == 'induced_lattice' and engine not in ['auto', 'latte', 'normaliz']:
@@ -7237,18 +7237,16 @@ class Polyhedron_base(Polyhedron_base1):
                 Latte().require()
                 engine = 'latte'
             except FeatureNotPresentError:
-                from sage.features.normaliz import PyNormaliz
                 try:
-                    PyNormaliz().require()
+                    PythonModule("PyNormaliz", spkg="pynormaliz").require()
                     engine = 'normaliz'
                 except FeatureNotPresentError:
                     raise RuntimeError("the induced rational measure can only be computed with the optional packages `latte_int`, or `pynormaliz`")
 
         if engine == 'auto' and measure == 'induced_lattice':
             # Enforce a default choice, change if a better engine is found.
-            from sage.features.normaliz import PyNormaliz
             try:
-                PyNormaliz().require()
+                PythonModule("PyNormaliz", spkg="pynormaliz").require()
                 engine = 'normaliz'
             except FeatureNotPresentError:
                 try:
comment:11

The version with from sage.features.normaliz import PyNormaliz is the better one. (from #27744)

kliem commented

Changed commit from 995ad00 to ff8c7ef

kliem commented

New commits:

6160076Merge branch 'u/gh-kliem/polyhedron_base3' of git://trac.sagemath.org/sage into u/gh-kliem/polyhedron_base4-new
ff8c7efmove methods relying on sage.graphs to Polyhedron_base4
kliem commented
comment:13

Replying to @mkoeppe:

The version with from sage.features.normaliz import PyNormaliz is the better one. (from #27744)

Yes, that is the version I based in upon. Wasn't easy. The merge conflict file was huge.

So what I did was pull u/gh-kliem/Polyhedron_base3 into develop. Then undid #27744 in base.py. Then I pulled u/gh-kliem/Polyhedron_base4. Redid #27744 and then squashed the three commits.

comment:14

Thanks, this looks good