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: | sage-9.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 follow-up: ↓ 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 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
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 follow-up: ↓ 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 follow-up: ↓ 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 ; follow-up: ↓ 22 Changed 5 years ago by
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 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 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.
comment:27 follow-up: ↓ 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 sage-7.6 to sage-wishlist
- 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_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
- 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_timeout-22576 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 <Ctrl-c> 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.