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

improve doctests coverage in graphs #30377

Closed
dcoudert opened this issue Aug 16, 2020 · 18 comments
Closed

improve doctests coverage in graphs #30377

dcoudert opened this issue Aug 16, 2020 · 18 comments

Comments

@dcoudert
Copy link
Contributor

Add doctests to reach 100% coverage in

  • graphs/generic_graph.py: was 99.1% (227 of 229)
  • graphs/graph_decompositions/fast_digraph.pyx: was 33.3% (1 of 3)
  • graphs/graph_list.py: was 77.8% (7 of 9)
  • graphs/tutte_polynomial.py: was 95.2% (20 of 21)

What remains:

  • graphs/graph_database.py: 93.3% (14 of 15). The missing doctest is in method _gen_interact_func which uses sagenb. So it will certainly be removed in the future.
  • graphs/graph_generators.py: 92.9% (13 of 14). The missing doctest is in method __append_to_doc used to feed global variable __doc__ used by sphinx. Don't know how to doctest that.

CC: @fchapoton

Component: graph theory

Author: David Coudert

Branch/Commit: eadfe03

Reviewer: Frédéric Chapoton, François Bissey

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

@dcoudert dcoudert added this to the sage-9.2 milestone Aug 16, 2020
@dcoudert
Copy link
Contributor Author

Branch: public/graphs/30377_doctests

@dcoudert
Copy link
Contributor Author

Commit: 1a4386a

@dcoudert

This comment has been minimized.

@dcoudert
Copy link
Contributor Author

comment:1

The added doctests allow to reach 100% coverage, but are not very useful by themself.


New commits:

8206e2dtrac #30377: improve generic_graph.py
6883f31trac #30377: graph_list.py and fast_digraph.pyx
1a4386atrac #30377: tutte_polynomial.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2020

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

d8955e0trac #30377: fix block syntax in fast_digraph.pyx

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2020

Changed commit from 1a4386a to d8955e0

@fchapoton
Copy link
Contributor

comment:3

ok, feu vert

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@dcoudert
Copy link
Contributor Author

comment:4

Thanks!

@kiwifb
Copy link
Member

kiwifb commented Aug 21, 2020

comment:5

Sorry, I am a bit annoyed already today.

Whose great idea was is to introduce new calls to installed_packages or any other of those functions in sage/misc/packages.py that we are desperately trying to purge to make sage more packageable?

It may be a lesser offence in a doctest than at runtime but it is still objectionable. If Volker keeps this ticket merged in the current state, be assured there will be a follow up ticket to remove those bits.

@dcoudert
Copy link
Contributor Author

comment:6

Sorry for that.

Should I change the doctest to something like that ?

sage: igraph_feature().is_present()  # optional - python_igraph 
True

or do you have a better suggestion ?

@kiwifb
Copy link
Member

kiwifb commented Aug 21, 2020

comment:7

I sincerely have no better suggestion. Playing with automagically discoverable dependency at runtime is always a bit fun. I notice some other python packages usually do a runtime detection at the beginning of the test run and then skip what is not available. sage is taking a very different approach.

And testing the runtime tester is always going to leave you in a funny position.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2020

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

a9aa831trac #30377: merged with 9.2.beta9
eadfe03trac #30377: avoid use of intalled_packages

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2020

Changed commit from d8955e0 to eadfe03

@dcoudert
Copy link
Contributor Author

comment:9

Should be better this way. The alternative is to simply remove the test.

@kiwifb
Copy link
Member

kiwifb commented Aug 23, 2020

Changed reviewer from Frédéric Chapoton to Frédéric Chapoton, François Bissey

@kiwifb
Copy link
Member

kiwifb commented Aug 23, 2020

comment:11

OK, Volker finally did not include the old version of the ticket in 9.2.beta10. Thanks for the correction.

@vbraun
Copy link
Member

vbraun commented Aug 26, 2020

Changed branch from public/graphs/30377_doctests to eadfe03

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

4 participants