Opened 5 years ago
Closed 13 months ago
#22576 closed defect (fixed)
Fix timeout in TransitiveGroups TestSuite
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  group theory  Keywords:  bug, doctest, days84 
Cc:  embray, slelievre  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  1b99249 (Commits, GitHub, GitLab)  Commit:  1b99249885a74fa775effb900b514d41f33c9ccc 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (31)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
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 followup: ↓ 5 Changed 5 years ago by
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
 Description modified (diff)
comment:5 in reply to: ↑ 3 Changed 5 years ago by
 Branch set to public/22576
 Commit set to 6236b0ddf2f4f7df58dcaac270fb496c7ab9d255
comment:6 Changed 5 years ago by
 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
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
this is caused by a stale GAP workspace. Do
sage c "gap_reset_workspace()"
comment:9 Changed 5 years ago by
 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 20170311101911a1e57781. 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
Don't we have # known bug
tag for such broken doctests?
comment:11 Changed 5 years ago by
 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

comment:12 Changed 5 years ago by
We do
comment:13 Changed 5 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
comment:14 Changed 5 years ago by
Thanks! I will reactivate database_gap
on my patchbot as soon as it is merged.
comment:15 followup: ↓ 17 Changed 5 years ago by
 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 followup: ↓ 18 Changed 5 years ago by
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 ; followup: ↓ 22 Changed 5 years ago by
comment:18 in reply to: ↑ 16 ; followup: ↓ 23 Changed 5 years ago by
Replying to jdemeyer:
Besides, I'm not really following. Do I understand you correctly that the proper fix is simply to run
gap_reset_workspace()
wheneverdatabase_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
 Status changed from needs_work to positive_review
comment:20 Changed 5 years ago by
 Description modified (diff)
comment:21 Changed 5 years ago by
 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
Replying to vdelecroix:
Replying to 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
Replying to vdelecroix:
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
 Description modified (diff)
comment:25 Changed 5 years ago by
 Stopgaps set to #22583
comment:26 Changed 5 years ago by
OK, so we need to deal with sage c "gap_reset_workspace()"
issue. As Jeroen rightly says on #22570, this cannot be done by spkginstall
, as it updates userspecific
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 timeconsuming 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.
comment:27 followup: ↓ 28 Changed 3 years ago by
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
 Cc embray slelievre added
 Component changed from packages: optional to packages: standard
 Description modified (diff)
 Milestone changed from sage7.6 to sagewishlist
 Summary changed from Timeout when database_gap is installed to Fix timeout in TransitiveGroups TestSuite
Replying to slelievre:
Is this still a bug after
database_gap
has been merged partly into thegap
and partly into thegap_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
 Branch set to public/groups/fix_transitive_groups_timeout22576
 Commit set to 1b99249885a74fa775effb900b514d41f33c9ccc
 Component changed from packages: standard to group theory
 Milestone changed from sagewishlist to sage9.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
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
LGTM, thanks!
comment:31 Changed 13 months ago by
 Branch changed from public/groups/fix_transitive_groups_timeout22576 to 1b99249885a74fa775effb900b514d41f33c9ccc
 Resolution set to fixed
 Status changed from positive_review to closed
T.an_element()
works. What doesn't work is creating an element of T from an element of T.The traceback after <Ctrlc> pointed me to these lines:
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:
No idea how to fix that, though.