Opened 4 months ago

Closed 4 months ago

#27245 closed enhancement (fixed)

py3: fix doctests in generic_graph (part 9) -- automorphism_group

Reported by: dcoudert 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) 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 months ago by dcoudert

  • Branch set to u/dcoudert/27245_automorphism_group
  • Commit set to 1595786bcad842cf413a979faed59194d8088a1f
  • Status changed from new to needs_review

New commits:

1595786trac #27245: doctests in automorphism_group

comment:2 follow-up: Changed 4 months ago by kdilks

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 months ago by git

  • Commit changed from 1595786bcad842cf413a979faed59194d8088a1f to b70159df97f416dc57d84e499c3b80122199f9e1

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

b70159dtrac #27245: reviewer's comments

comment:4 in reply to: ↑ 2 Changed 4 months ago by dcoudert

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 months ago by kdilks

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

comment:6 Changed 4 months ago by git

  • Commit changed from b70159df97f416dc57d84e499c3b80122199f9e1 to ae16cbab88e7e5f190665d0b0c26fe261eade6ce

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

ae16cbatrac #27245: better doctest

comment:7 Changed 4 months ago by dcoudert

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

comment:8 Changed 4 months ago by kdilks

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 months ago by dcoudert

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 months ago by kdilks

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 months ago by git

  • Commit changed from ae16cbab88e7e5f190665d0b0c26fe261eade6ce to 218fcb4d3f639a93b375ace614d530c23b0c1cbe

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

218fcb4trac #27245: at least robust doctest

comment:12 Changed 4 months ago by dcoudert

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

comment:13 Changed 4 months ago by dcoudert

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 months ago by kdilks

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

comment:15 Changed 4 months ago by kdilks

  • Reviewers set to Kevin Dilks
  • Status changed from needs_review to positive_review

comment:16 Changed 4 months ago by dcoudert

Thank you.

comment:17 Changed 4 months ago by vbraun

  • Branch changed from u/dcoudert/27245_automorphism_group to 218fcb4d3f639a93b375ace614d530c23b0c1cbe
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.