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:

Status badges

Description (last modified by slelievre)

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 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: 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:

c28fcf018220: raise an error for non exact fields
6236b0d22576: 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:

287dfb622576: 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()"

I suppose this will be fixed good and proper in #22570.

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

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:

304355c22576: disable two tests

comment:12 Changed 5 years ago by vdelecroix

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: 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: 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: Changed 5 years ago by 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.

#22583

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

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() 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

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.

#22583

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

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 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 package was installed? (otherwise this looks like a time-consuming check).

Version 0, edited 5 years ago by dimpase (next)

comment:27 follow-up: 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

  • 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 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:

1b99249Working 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.