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

py3: fix doctest with igraph #27628

Closed
dcoudert opened this issue Apr 9, 2019 · 23 comments
Closed

py3: fix doctest with igraph #27628

dcoudert opened this issue Apr 9, 2019 · 23 comments

Comments

@dcoudert
Copy link
Contributor

dcoudert commented Apr 9, 2019

ordering issue.

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

CC: @dimpase

Component: graph theory

Author: David Coudert

Branch: 1e773f5

Reviewer: Frédéric Chapoton, Dima Pasechnik

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

@dcoudert dcoudert added this to the sage-8.8 milestone Apr 9, 2019
@dcoudert
Copy link
Contributor Author

dcoudert commented Apr 9, 2019

Commit: 8ee4a7a

@dcoudert
Copy link
Contributor Author

dcoudert commented Apr 9, 2019

New commits:

8ee4a7atrac #27628: doctest with igraph

@dcoudert
Copy link
Contributor Author

dcoudert commented Apr 9, 2019

Branch: public/graphs/27628_igraph_graph

@fchapoton
Copy link
Contributor

comment:2

ok, looks good enough

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@dcoudert
Copy link
Contributor Author

dcoudert commented Apr 9, 2019

comment:3

Thank you Frederic for this super fast review ;)

@vbraun
Copy link
Member

vbraun commented Apr 11, 2019

comment:4

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
**********************************************************************

@dcoudert
Copy link
Contributor Author

comment:5

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 ?

@dcoudert
Copy link
Contributor Author

comment:6

Can we set this ticket back to positive review ?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2019

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2019

Changed commit from 8ee4a7a to 1e773f5

@dcoudert
Copy link
Contributor Author

comment:8

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

@dcoudert

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Apr 23, 2019

comment:10

looks good to me.

@dimpase
Copy link
Member

dimpase commented Apr 23, 2019

Changed reviewer from Frédéric Chapoton to Frédéric Chapoton, Dima Pasechnik

@kiwifb
Copy link
Member

kiwifb commented Apr 27, 2019

comment:11

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.

@dimpase
Copy link
Member

dimpase commented Apr 27, 2019

comment:12

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.

@kiwifb
Copy link
Member

kiwifb commented Apr 27, 2019

comment:13

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.

@vbraun
Copy link
Member

vbraun commented Apr 27, 2019

Changed branch from public/graphs/27628_igraph_graph to 1e773f5

@fchapoton
Copy link
Contributor

Changed commit from 1e773f5 to none

@fchapoton
Copy link
Contributor

comment:15

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

@dimpase
Copy link
Member

dimpase commented May 11, 2019

comment:16

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}

@dcoudert
Copy link
Contributor Author

comment:17

it's what we do in #27811 ;)

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

5 participants