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: sage-8.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:

Status badges

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 David Coudert

Branch: u/dcoudert/27245_automorphism_group
Commit: 1595786bcad842cf413a979faed59194d8088a1f
Status: newneeds_review

New commits:

1595786trac #27245: doctests in automorphism_group

comment:2 Changed 4 years ago by Kevin Dilks

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 git

Commit: 1595786bcad842cf413a979faed59194d8088a1fb70159df97f416dc57d84e499c3b80122199f9e1

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

b70159dtrac #27245: reviewer's comments

comment:4 in reply to:  2 Changed 4 years ago by David Coudert

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 Kevin Dilks

Why not make it Graph({'a':['a'], 'b':[]}).automorphism_group().domain() == {'a', 'b'} returns True ?

comment:6 Changed 4 years ago by git

Commit: b70159df97f416dc57d84e499c3b80122199f9e1ae16cbab88e7e5f190665d0b0c26fe261eade6ce

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

ae16cbatrac #27245: better doctest

comment:7 Changed 4 years ago by David Coudert

Agreed. It better shows that the output is a set. Thanks.

comment:8 Changed 4 years ago by Kevin Dilks

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 David Coudert

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 Kevin Dilks

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 git

Commit: ae16cbab88e7e5f190665d0b0c26fe261eade6ce218fcb4d3f639a93b375ace614d530c23b0c1cbe

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

218fcb4trac #27245: at least robust doctest

comment:12 Changed 4 years ago by David Coudert

So let's go back to something that is working. Thanks.

comment:13 Changed 4 years ago by David Coudert

I just tried over 8.7.beta5 and it's working well. So can we set this ticket to positive review ?

comment:14 Changed 4 years ago by Kevin Dilks

Trying to get my computer updated to 8.7b5 so I can double-check.

comment:15 Changed 4 years ago by Kevin Dilks

Reviewers: Kevin Dilks
Status: needs_reviewpositive_review

comment:16 Changed 4 years ago by David Coudert

Thank you.

comment:17 Changed 4 years ago by Volker Braun

Branch: u/dcoudert/27245_automorphism_group218fcb4d3f639a93b375ace614d530c23b0c1cbe
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.