Opened 4 months ago

Closed 4 months ago

Last modified 3 months ago

#27628 closed enhancement (fixed)

py3: fix doctest with igraph

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.8
Component: graph theory Keywords: py3, graph
Cc: dimpase Merged in:
Authors: David Coudert Reviewers: Frédéric Chapoton, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 1e773f5 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by dcoudert)

ordering issue.

Also change the way to check if igraph is installed to avoid a recurrent pyflakes error.

Change History (17)

comment:1 Changed 4 months ago by dcoudert

  • Branch set to public/graphs/27628_igraph_graph
  • Commit set to 8ee4a7aab47099b5586311d36ded3c45076eee6c
  • Status changed from new to needs_review

New commits:

8ee4a7atrac #27628: doctest with igraph

comment:2 Changed 4 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, looks good enough

comment:3 Changed 4 months ago by dcoudert

Thank you Frederic for this super fast review ;)

comment:4 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work

There are a couple of failures of teh sort

**********************************************************************
File "src/sage/graphs/generic_graph.py", line 9581, in sage.graphs.generic_graph.GenericGraph.?
Failed example:
    G.pagerank(algorithm="igraph")
Exception raised:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.graphs.generic_graph.GenericGraph.?[10]>", line 1, in <module>
        G.pagerank(algorithm="igraph")
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/graphs/generic_graph.py", line 9668, in pagerank
        raise PackageNotFoundError("igraph")
    PackageNotFoundError: the package 'igraph' was not found. You can install it by running 'sage -i igraph' in a shell
**********************************************************************

comment:5 Changed 4 months ago by dcoudert

It's not due to this ticket (nothing to do with pagerank) but to #27480 which adds pagerank. We forgot to tag doctests as # optional - python_igraph, and I believe it's too late so we must wait for next beta, right ?

comment:6 Changed 4 months ago by dcoudert

  • Status changed from needs_work to needs_review

Can we set this ticket back to positive review ?

comment:7 Changed 4 months ago by git

  • Commit changed from 8ee4a7aab47099b5586311d36ded3c45076eee6c to 1e773f5bfbd2fbe0712c7fe0e1731eeb7d664dc5

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

e6051f5trac #27628: Merged with 8.8.beta3
1e773f5trac #27628: fix pyflakes issue with igraph

comment:8 Changed 4 months ago by dcoudert

I changed the way we check if igraph is installed to avoid a pyflakes error.

comment:9 Changed 4 months ago by dcoudert

  • Cc dimpase added
  • Description modified (diff)

comment:10 Changed 4 months ago by dimpase

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me.

comment:11 Changed 4 months ago by fbissey

Well, let's do a follow up ticket since this is getting merged properly right now. Please refrain to introduce anymore call to is_package_installed, especially when checking for a python package. There is a proper python way to do this, like in the other sections of code using igraph- from sage/graphs/digraph.py line 693 onward

            try:
                import igraph
            except ImportError:
                raise ImportError(...)

replacing the ImportError(...) with PackageNotFoundError is acceptable.

is_package_installed is a big problem for sage-on-distro and it is a filthy quick and dirty hack that shouldn't ever have been included.

comment:12 Changed 4 months ago by dimpase

how about a ticket for getting rid of is_package_installed()? Or at least making sure it's not used on importable things?

I wonder what pyflakes error one sees on a seemingly good try/except.

comment:13 Changed 4 months ago by fbissey

It is already on its way out. The only place it is not replaced by feature or something else at runtime is in sage/databases/cremona.py and there is a ticket for that.

It is currently impossible to totally remove it. doctesting with --option=all relies on it (or other features of packages.py which is just as bad) :( and building with some optional package still depends on it look in sage_setup/optional_extension.py . In both case replacement are not easy.

comment:14 Changed 4 months ago by vbraun

  • Branch changed from public/graphs/27628_igraph_graph to 1e773f5bfbd2fbe0712c7fe0e1731eeb7d664dc5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 3 months ago by chapoton

  • Commit 1e773f5bfbd2fbe0712c7fe0e1731eeb7d664dc5 deleted

There is an issue with igraph on the arando patchbot with 8.8.b4 and 8.8.b5:

**********************************************************************
File "src/sage/graphs/generic_graph.py", line 9663, in sage.graphs.generic_graph.GenericGraph.?
Failed example:
    G.pagerank(alpha=0.50, algorithm="igraph")  # optional - python_igraph
Expected:
    {0: 0.25, 1: 0.25, 2: 0.24999999999999997, 3: 0.24999999999999997}
Got:
    {0: 0.25, 1: 0.25, 2: 0.25, 3: 0.25}
**********************************************************************
1 item had failures:
   1 of 860 in sage.graphs.generic_graph.GenericGraph.?
    [3497 tests, 1 failure, 50.96 s]
----------------------------------------------------------------------
sage -t --long src/sage/graphs/generic_graph.py  # 1 doctest failed

comment:16 Changed 3 months ago by dimpase

one can use a tag like # abs tol 1e-12 and make the data

    {0: 0.25, 1: 0.25, 2: 0.25, 3: 0.25}

comment:17 Changed 3 months ago by dcoudert

it's what we do in #27811 ;)

Note: See TracTickets for help on using tickets.