Opened 4 years ago
Closed 4 years ago
#27245 closed enhancement (fixed)
py3: fix doctests in generic_graph (part 9)  automorphism_group
Reported by:  David Coudert  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  graph theory  Keywords:  py3, graph 
Cc:  Merged in:  
Authors:  David Coudert  Reviewers:  Kevin Dilks 
Report Upstream:  N/A  Work issues:  
Branch:  218fcb4 (Commits, GitHub, GitLab)  Commit:  218fcb4d3f639a93b375ace614d530c23b0c1cbe 
Dependencies:  Stopgaps: 
Description
We fix failing doctests in automorphism_group
.
We also do some minor cleaning in the method.
Change History (17)
comment:1 Changed 4 years ago by
Branch:  → u/dcoudert/27245_automorphism_group 

Commit:  → 1595786bcad842cf413a979faed59194d8088a1f 
Status:  new → needs_review 
comment:2 followup: 4 Changed 4 years ago by
In labelled automorphism group test, why not have d = digraphs.DeBruijn(3,2)
at the very beginning?
In the check for correct domain in the empty group case, is there any reason for changing the output from a set to a sorted list?
comment:3 Changed 4 years ago by
Commit:  1595786bcad842cf413a979faed59194d8088a1f → b70159df97f416dc57d84e499c3b80122199f9e1 

Branch pushed to git repo; I updated commit sha1. New commits:
b70159d  trac #27245: reviewer's comments

comment:4 Changed 4 years ago by
Replying to kdilks:
In labelled automorphism group test, why not have
d = digraphs.DeBruijn(3,2)
at the very beginning?
Right, done.
In the check for correct domain in the empty group case, is there any reason for changing the output from a set to a sorted list?
In py2 we always get {'a', 'b'}
, but in py3 I get {'b', 'a'}
. So I proposed to used a sorted list to ensure that we always get the same result. Alternatives are to tag doctests as # py2
or # py3
, or to change the test to check equality with the set {'a', 'b'}
, etc. I don't know which is the best option here.
comment:5 Changed 4 years ago by
Why not make it Graph({'a':['a'], 'b':[]}).automorphism_group().domain() == {'a', 'b'}
returns True
?
comment:6 Changed 4 years ago by
Commit:  b70159df97f416dc57d84e499c3b80122199f9e1 → ae16cbab88e7e5f190665d0b0c26fe261eade6ce 

Branch pushed to git repo; I updated commit sha1. New commits:
ae16cba  trac #27245: better doctest

comment:8 Changed 4 years ago by
It looks like the test doesn't work this way, presumably because ag.domain()
is <class 'sage.sets.finite_enumerated_set.FiniteEnumeratedSet_with_category'>
, and {'a', 'b'}
is just <type 'set'>
.
comment:9 Changed 4 years ago by
sage: from sage.sets.finite_enumerated_set import FiniteEnumeratedSet sage: F = FiniteEnumeratedSet(['a', 'b']) sage: F {'a', 'b'} sage: type(F) <class 'sage.sets.finite_enumerated_set.FiniteEnumeratedSet_with_category'> sage: ag = Graph({'a':['a'], 'b':[]}).automorphism_group() sage: ag.domain() {'b', 'a'} sage: type(ag.domain()) <class 'sage.sets.finite_enumerated_set.FiniteEnumeratedSet_with_category'> sage: ag.domain() == F False
So I propose to return to my previous proposal unless you have a better solution.
comment:10 Changed 4 years ago by
I'm fine with the previous proposal. It looks like domain()
really needs to be a FiniteEnumeratedSet?() to play nicely with GAP.
comment:11 Changed 4 years ago by
Commit:  ae16cbab88e7e5f190665d0b0c26fe261eade6ce → 218fcb4d3f639a93b375ace614d530c23b0c1cbe 

Branch pushed to git repo; I updated commit sha1. New commits:
218fcb4  trac #27245: at least robust doctest

comment:13 Changed 4 years ago by
I just tried over 8.7.beta5 and it's working well. So can we set this ticket to positive review ?
comment:15 Changed 4 years ago by
Reviewers:  → Kevin Dilks 

Status:  needs_review → positive_review 
comment:17 Changed 4 years ago by
Branch:  u/dcoudert/27245_automorphism_group → 218fcb4d3f639a93b375ace614d530c23b0c1cbe 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
trac #27245: doctests in automorphism_group