Opened 13 months ago
Closed 12 months ago
#27245 closed enhancement (fixed)
py3: fix doctests in generic_graph (part 9)  automorphism_group
Reported by:  dcoudert  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)  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 13 months ago by
 Branch set to u/dcoudert/27245_automorphism_group
 Commit set to 1595786bcad842cf413a979faed59194d8088a1f
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 13 months 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 13 months ago by
 Commit changed from 1595786bcad842cf413a979faed59194d8088a1f to b70159df97f416dc57d84e499c3b80122199f9e1
Branch pushed to git repo; I updated commit sha1. New commits:
b70159d  trac #27245: reviewer's comments

comment:4 in reply to: ↑ 2 Changed 13 months 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 13 months ago by
Why not make it Graph({'a':['a'], 'b':[]}).automorphism_group().domain() == {'a', 'b'}
returns True
?
comment:6 Changed 13 months ago by
 Commit changed from b70159df97f416dc57d84e499c3b80122199f9e1 to ae16cbab88e7e5f190665d0b0c26fe261eade6ce
Branch pushed to git repo; I updated commit sha1. New commits:
ae16cba  trac #27245: better doctest

comment:7 Changed 13 months ago by
Agreed. It better shows that the output is a set. Thanks.
comment:8 Changed 13 months 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 13 months 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 13 months 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 13 months ago by
 Commit changed from ae16cbab88e7e5f190665d0b0c26fe261eade6ce to 218fcb4d3f639a93b375ace614d530c23b0c1cbe
Branch pushed to git repo; I updated commit sha1. New commits:
218fcb4  trac #27245: at least robust doctest

comment:12 Changed 13 months ago by
So let's go back to something that is working. Thanks.
comment:13 Changed 12 months ago by
I just tried over 8.7.beta5 and it's working well. So can we set this ticket to positive review ?
comment:14 Changed 12 months ago by
Trying to get my computer updated to 8.7b5 so I can doublecheck.
comment:15 Changed 12 months ago by
 Reviewers set to Kevin Dilks
 Status changed from needs_review to positive_review
comment:16 Changed 12 months ago by
Thank you.
comment:17 Changed 12 months ago by
 Branch changed from u/dcoudert/27245_automorphism_group to 218fcb4d3f639a93b375ace614d530c23b0c1cbe
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #27245: doctests in automorphism_group