Opened 5 years ago

Closed 13 months ago

# Fix timeout in TransitiveGroups TestSuite

Reported by: Owned by: vdelecroix major sage-9.4 group theory bug, doctest, days84 embray, slelievre Travis Scrimshaw Dima Pasechnik N/A 1b99249 1b99249885a74fa775effb900b514d41f33c9ccc

The test suite of `src/sage/groups/perm_gps/permgroup_named.py` takes an infinite amount of time.

The responsible doctest is

```(line 1924):    TestSuite(TransitiveGroups()).run()
```

And it is blocked because of

```sage: T = TransitiveGroups()
sage: elt = T.an_element()
sage: T(elt)     # hangs
```

This comes from #22382.

### comment:1 Changed 5 years ago by SimonKing

`T.an_element()` works. What doesn't work is creating an element of T from an element of T.

The traceback after <Ctrl-c> pointed me to these lines:

```/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/sets/disjoint_union_enumerated_sets.py in _element_constructor_facade(self, el)
575             return el
576
--> 577         for P in self._family:
578             try:
579                 return P(el)
```

sage: T._family Lazy family (<lambda>(i))_{i in Non negative integers} }}}

It is an infinite family - of course it hangs.

Obvious solution: First check whether the parent of the given element belongs to the family. But alas, the element of T has no reasonable parent at all. I get the following rather strange sequence of lines:

```sage: t = T.an_element()
sage: t.parent() in T._family
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-14-7530d2cfb513> in <module>()
----> 1 t.parent() in T._family

/home/king/Sage/git/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.__contains__ (/home/king/Sage/git/sage/src/build/cythonized/sage/structure/parent.c:10458)()
1140             False
1141         """
-> 1142         P = parent(x)
1143         if P is self or P == self:
1144             return True

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/element.pxd in sage.structure.element.parent (/home/king/Sage/git/sage/src/build/cythonized/sage/structure/parent.c:28336)()
71         return type(x)
72     else:
---> 73         return p()
74
75

TypeError: descriptor 'parent' of 'sage.structure.sage_object.SageObject' object needs an argument
sage: t
Transitive group number 1 of degree 0
sage: t.parent()
<class 'sage.groups.perm_gps.permgroup_named.TransitiveGroup_with_category'>
```

No idea how to fix that, though.

### comment:2 Changed 5 years ago by dimpase

I don't know what exactly `family` means in this context, and perhaps I talk nonsense, but shouldn't iteration through such things be handled by an `iterator` (and `yield`)?

### comment:3 follow-up: ↓ 5 Changed 5 years ago by tscrim

So this comes from #22382, where we actually are now doing some validity checks. So when the family of sets in the union is infinite, we get the hanging behavior when an element is not in the set. We can reverse this behavior for this case, but that is a somewhat separate issue as in this case, the actual parent of the element in question is broken.

### comment:4 Changed 5 years ago by vdelecroix

• Description modified (diff)

### comment:5 in reply to: ↑ 3 Changed 5 years ago by vdelecroix

• Branch set to public/22576
• Commit set to 6236b0ddf2f4f7df58dcaac270fb496c7ab9d255

New commits:

 ​c28fcf0 `18220: raise an error for non exact fields` ​6236b0d `22576: disable two tests`

### comment:6 Changed 5 years ago by git

• Commit changed from 6236b0ddf2f4f7df58dcaac270fb496c7ab9d255 to 287dfb6d16d36e53dcacd844ce877adbefb7ba2a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​287dfb6 `22576: disable two tests`

### comment:7 Changed 5 years ago by vdelecroix

After disabling two tests I am able to run the doctests. However, `database_gap` seems to be broken (at least on my computer)

```\$ sage -f database_gap
\$ sage -c "PrimitiveGroups(12).cardinality()"
verbose 0 (2474: permgroup_named.py, cardinality) Warning: PrimitiveGroups requires the GAP database package. Please install it with ``sage -i database_gap``.
```

### comment:8 Changed 5 years ago by dimpase

this is caused by a stale GAP workspace. Do

```sage -c "gap_reset_workspace()"
```
Version 0, edited 5 years ago by dimpase (next)

### comment:9 Changed 5 years ago by vdelecroix

• Authors set to Vincent Delecroix
• Status changed from new to needs_review

Indeed

```\$ sage -t --long --optional=sage,database_gap permgroup_named.py
too few successful tests, not using stored timings
Running doctests with ID 2017-03-11-10-19-11-a1e57781.
Git branch: develop
Using --optional=database_gap,sage
Doctesting 1 file.
sage -t --long permgroup_named.py
[445 tests, 3.90 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4.0 seconds
cpu time: 3.2 seconds
cumulative wall time: 3.9 seconds
```

### comment:10 Changed 5 years ago by dimpase

Don't we have `# known bug` tag for such broken doctests?

### comment:11 Changed 5 years ago by git

• Commit changed from 287dfb6d16d36e53dcacd844ce877adbefb7ba2a to 304355cbf5b8b71ddacd6b188f1549cafb49c9af

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​304355c `22576: disable two tests`

We do

### comment:13 Changed 5 years ago by dimpase

• Reviewers set to Dima Pasechnik
• Status changed from needs_review to positive_review

### comment:14 Changed 5 years ago by vdelecroix

Thanks! I will reactivate `database_gap` on my patchbot as soon as it is merged.

### comment:15 follow-up: ↓ 17 Changed 5 years ago by jdemeyer

• Status changed from positive_review to needs_work

I think the proper way to deal with `# known bug` is to open a new ticket to add `# known bug` and keep this ticket open for the real issue.

### comment:16 follow-up: ↓ 18 Changed 5 years ago by jdemeyer

Besides, I'm not really following. Do I understand you correctly that the proper fix is simply to run `gap_reset_workspace()` whenever `database_gap` is installed? So it's unrelated to #22382 after all?

### comment:17 in reply to: ↑ 15 ; follow-up: ↓ 22 Changed 5 years ago by vdelecroix

I think the proper way to deal with `# known bug` is to open a new ticket to add `# known bug` and keep this ticket open for the real issue.

### comment:18 in reply to: ↑ 16 ; follow-up: ↓ 23 Changed 5 years ago by vdelecroix

Besides, I'm not really following. Do I understand you correctly that the proper fix is simply to run `gap_reset_workspace()` whenever `database_gap` is installed? So it's unrelated to #22382 after all?

You are not really understanding. This bug of this ticket is disjoint from the `gap_reset_workspace()` which has to do with 7.

### comment:19 Changed 5 years ago by vdelecroix

• Status changed from needs_work to positive_review

### comment:20 Changed 5 years ago by vdelecroix

• Description modified (diff)

### comment:21 Changed 5 years ago by jdemeyer

• Authors Vincent Delecroix deleted
• Branch public/22576 deleted
• Commit 304355cbf5b8b71ddacd6b188f1549cafb49c9af deleted
• Priority changed from blocker to major
• Reviewers Dima Pasechnik deleted
• Status changed from positive_review to needs_work

### comment:22 in reply to: ↑ 17 Changed 5 years ago by jdemeyer

I think the proper way to deal with `# known bug` is to open a new ticket to add `# known bug` and keep this ticket open for the real issue.

Better do it the other way around because all the discussion is happening here.

### comment:23 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

You are not really understanding.

Well, the comments were not clear either. It was not obvious to me that the "this" from 8 was referring not the bug on this ticket.

### comment:24 Changed 5 years ago by jdemeyer

• Description modified (diff)

### comment:25 Changed 5 years ago by jdemeyer

• Stopgaps set to #22583

### comment:26 Changed 5 years ago by dimpase

OK, so we need to deal with `sage -c "gap_reset_workspace()"` issue. As Jeroen rightly says on #22570, this cannot be done by `spkg-install`, as it updates user-specific stuff in `~/.sage/gap/`.

What else can be done? I only see an option of `sage` checking on startup whether `sage -c "gap_reset_workspace()"` must be run. Can we get away with only triggering it if a new Sage package was installed? (otherwise this looks like a time-consuming check).

Docs on how to install GAP packages manually should also indicate the need to run `sage -c "gap_reset_workspace()"` after a GAP package is installed or removed.

Last edited 5 years ago by dimpase (previous) (diff)

### comment:27 follow-up: ↓ 28 Changed 3 years ago by slelievre

Is this still a bug after `database_gap` has been merged partly into the `gap` and partly into the `gap_packages` packages?

### comment:28 in reply to: ↑ 27 Changed 14 months ago by slelievre

• Component changed from packages: optional to packages: standard
• Description modified (diff)
• Milestone changed from sage-7.6 to sage-wishlist
• Summary changed from Timeout when database_gap is installed to Fix timeout in TransitiveGroups TestSuite

Is this still a bug after `database_gap` has been merged partly into the `gap` and partly into the `gap_packages` packages?

Answering my own question: yes, the following still hangs:

```sage: T = TransitiveGroups()
sage: t = T.an_element()
sage: T(t)
```

### comment:29 Changed 13 months ago by tscrim

• Authors set to Travis Scrimshaw
• Branch set to public/groups/fix_transitive_groups_timeout-22576
• Commit set to 1b99249885a74fa775effb900b514d41f33c9ccc
• Component changed from packages: standard to group theory
• Milestone changed from sage-wishlist to sage-9.4
• Status changed from needs_work to needs_review
• Stopgaps #22583 deleted

Here is a fix. The issue is that `TransitiveGroups` claims to be a `Parent`, but it is not as it doesn't have proper elements. I just hacked around that by directly implementing a `__call__`. It has nothing to do with the GAP package.

New commits:

 ​1b99249 `Working around not being a Parent for TransitiveGroups.`

### comment:30 Changed 13 months ago by dimpase

• Reviewers set to Dima Pasechnik
• Status changed from needs_review to positive_review

LGTM, thanks!

### comment:31 Changed 13 months ago by vbraun

• Branch changed from public/groups/fix_transitive_groups_timeout-22576 to 1b99249885a74fa775effb900b514d41f33c9ccc
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.