Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#10335 closed enhancement (fixed)

Add domains for permutation groups

Reported by: mhansen Owned by: joyner
Priority: major Milestone: sage-4.7.2
Component: group theory Keywords: sd31
Cc: sage-combinat, jasonbhill, rbeezer, jdemeyer Merged in: sage-4.7.2.alpha3
Authors: Mike Hansen, Jason Hill, David Loeffler Reviewers: Robert Miller, Rob Beezer, Nicolas Borie, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10334 Stopgaps:

Description (last modified by davidloeffler)

Often, one wants to define a permutation group on a set other than {1,2,...,n} such as say {'a', 'b', 'c', 'd'} .

For example,

sage: G = PermutationGroup([ [('c','d')], [('a','c')] ])
sage: G.orbit('a')
['a', 'c', 'd']

Apply:

  1. trac_10335-permgroup_domain-rebase.patch

Attachments (7)

trac_10335-permgroup_domain-mh.patch (130.5 KB) - added by mhansen 8 years ago.
trac_10335_german_tutorial_doc.patch (1.4 KB) - added by rbeezer 8 years ago.
trac_10335-permgroup_domain-import_loop_fix-nt.patch (1.4 KB) - added by nthiery 8 years ago.
Merge only if needed
trac_10335-tutorial-languages-fixes.patch (2.5 KB) - added by rbeezer 8 years ago.
German and Russian language fixes combined
trac_10335-solaris_issue.patch (894 bytes) - added by davidloeffler 8 years ago.
apply over previous patches
trac_10335-updates.patch (2.2 KB) - added by davidloeffler 8 years ago.
apply over previous patches
trac_10335-permgroup_domain-rebase.patch (133.5 KB) - added by davidloeffler 8 years ago.
patch against 4.7.2.alpha3 prerelease

Download all attachments as: .zip

Change History (87)

comment:1 Changed 9 years ago by mhansen

  • Description modified (diff)
  • Status changed from new to needs_review

This depends

comment:2 Changed 9 years ago by mhansen

  • Cc sage-combinat jasonbhill added

comment:3 Changed 9 years ago by rlm

  • Reviewers set to Robert Miller
  • Work issues set to needs rebase

Applying to sage-4.6.2.alpha0 on top of #10334, I get:

applying trac_10335-permgroup_domain-mh.patch
patching file sage/groups/perm_gps/permgroup.py
Hunk #5 FAILED at 321
1 out of 59 hunks FAILED -- saving rejects to file sage/groups/perm_gps/permgroup.py.rej
patching file sage/groups/perm_gps/permgroup_named.py
Hunk #10 succeeded at 313 with fuzz 2 (offset 0 lines).
Hunk #11 succeeded at 315 with fuzz 1 (offset -49 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_10335-permgroup_domain-mh.patch

The failure:

$ cat sage/groups/perm_gps/permgroup.py.rej
--- permgroup.py
+++ permgroup.py
@@ -320,7 +322,7 @@
         sage: G = PermutationGroup([[(1,2,3),(4,5)],[(3,4)]])
         sage: TestSuite(G).run()
     """
-    def __init__(self, gens=None, gap_group=None, canonicalize=True, category = None):
+    def __init__(self, gens=None, gap_group=None, canonicalize=True, category=None, domain=None):
         r"""
         INPUT:

Looks trivial to fix, and I'm excited to see this feature in Sage. Thank you, Mike!

comment:4 Changed 9 years ago by rlm

  • Description modified (diff)

comment:5 Changed 9 years ago by nthiery

  • Work issues needs rebase deleted

This was most likely a minor conflict with Rob's permgroup patches which were merged late in the 4.6.2 release cycle.

Only apply: trac_10335-permgroup_domain-mh.2.patch

comment:6 Changed 9 years ago by nthiery

  • Status changed from needs_review to needs_work

Ah, the test fail, and that's because the patch still did not commute perfectly with trac_8359-coxeter-groups-permutation-nt.patch. we need to have this patch add the category argument to init of permutation groups, and remove that part from 8359.

Mike: let me know if you can do it shortly, and then #8359 is fair game. Otherwise I'll handle it.

comment:7 Changed 9 years ago by rbeezer

  • Cc rbeezer added

comment:8 Changed 9 years ago by mhansen

  • Description modified (diff)

I've fixed the issue along with updating the patch at #8359.

Only apply: trac_10335-permgroup_domain-mh.patch

comment:9 Changed 9 years ago by rbeezer

This looks real good, Mike. I spent some time with this today. I know its not done yet, but hopefully the following will be helpful.

I applied the patch from sage-combinat for #8359 to 4.7.alpha1, as best I could tell (used patch from http://combinat.sagemath.org/hgwebdir.cgi/patches/rev/508d4942a6d2). One hunk failure at sage/combinat/root_system/all.py. Then #10334 had a hunk failure in sage/groups/perm_gps/permgroup.py and #10335 had a had failure in sage/categories/pushout.py. All easy to fix by hand, and maybe a result of not getting the right thing from the sage-combinat server.

Passed all tests in sage/groups/perm_gps, except two, so not too far off. Both doctest failures look very minor, and a result of routines I wrote that have been added recently.

**********************************************************************
File "/sage/dev/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 1920:
    sage: G.subgroups()
Expected:
    [Permutation Group with generators [()],
     Permutation Group with generators [(2,3)],
     Permutation Group with generators [(1,2)],
     Permutation Group with generators [(1,3)],
     Permutation Group with generators [(1,2,3)],
     Permutation Group with generators [(1,3,2), (1,2)]]
Got:
    [Permutation Group with generators [()],
     Permutation Group with generators [(2,3)],
     Permutation Group with generators [(1,2)],
     Permutation Group with generators [(1,3)],
     Permutation Group with generators [(1,2,3)],
     Permutation Group with generators [(1,2), (1,3,2)]]
**********************************************************************
File "/sage/dev/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 2082:
    sage: A.cosets(S)
Expected:
    Traceback (most recent call last):
    ...
    ValueError: Subgroup of SymmetricGroup(3) generated by [(1,2)] is not a subgroup of AlternatingGroup(3)
Got:
    Traceback (most recent call last):
      File "/sage/dev/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/sage/dev/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/sage/dev/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_61[29]>", line 1, in <module>
        A.cosets(S)###line 2082:
    sage: A.cosets(S)
      File "/sage/dev/local/lib/python/site-packages/sage/groups/perm_gps/permgroup.py", line 2099, in cosets
        raise ValueError("%s is not a subgroup of %s" % (S, self))
    ValueError: Subgroup of (Symmetric group of order 3! as a permutation group) generated by [(1,2)] is not a subgroup of Alternating group of order 3!/2 as a permutation group
**********************************************************************

I think the stabilizer() routine needs some work.

try:
    postition = self._domain_to_gap[point]
except KeyError:
    return self.subgroup(gens=self.gens())
return self.subgroup(gap_group=gap.Stabilizer(self, point))

"position" is mis-spelled as first defined, and in the return, I think "point" should be replaced with "position". With these changes, this seems to work properly with a non-trivial domain in play.

Not a comprehensive look, but hopefully of use. Let me know what I can do to help (including assisting with documentation). It'd be nice if there was a patch, or something, at #8359 explaining how best to apply that work.

Rob

comment:10 follow-up: Changed 9 years ago by nthiery

Hi Mike,

For the record: I rebased trac_10335-permgroup_domain-mh.patch on the sage-combinat server on top of #9949

comment:11 in reply to: ↑ 10 Changed 8 years ago by nthiery

Hi Mike!

Replying to nthiery:

For the record: I rebased trac_10335-permgroup_domain-mh.patch on the sage-combinat server on top of #9949

With Rob we are both at Sage days 30, and would love to push permutation groups in. Rob is going to rebase #10334, and I'll finalize the review. #10335 itself needs rebase on 3.4.7, and has quite a few failing doctests, though most look harmless (it's just that the output of permutation groups have changed). Would there be any chance for you to work on this early this week?

Thanks!

Cheers,

Nicolas

comment:12 Changed 8 years ago by mhansen

Sure -- I'll try to get this taken care of tonight.

comment:13 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review

I've fixed the doctest failures and updated the patch.

comment:14 Changed 8 years ago by rbeezer

  • Status changed from needs_review to needs_work

I ran make ptestlong and got quite a few failures. I even tried it twice - not sure what is up.

  • Many were in sage/doc
  • Many were simply the new version of _repr_ for the symmetric group, and its subgroups
  • The latex() method is putting a comma between the cycles within each generator. Is this desired?
    return ",".join(["(" + ",".join([latex(x) for x in cycle])+")" for cycle in self.cycle_tuples()])
    
    At a minimum it is causing doctest failures.
  • 73 failures in sage/rings/number_field/galois_group.py, likely all the same root cause

Saw these in the patch itself:

  • a stray "deprecated_function_alias" in a docstring
  • ``selfs`` in a docstring

comment:15 Changed 8 years ago by nborie

  • Dependencies set to #10334

comment:16 Changed 8 years ago by mhansen

  • Status changed from needs_work to needs_review

I'm pretty sure that this patches addresses all of the above problems.

Apply trac_10335-permgroup_domain-mh.patch

comment:17 Changed 8 years ago by mhansen

Fixed commit message.

Apply trac_10335-permgroup_domain-mh.patch

comment:18 Changed 8 years ago by nborie

I was very close to set a positive review for this patch but I got this faillure :

nicolas@lancelot:/opt/sage/devel/sage-combinat$ sage -t sage/groups/perm_gps/permgroup.py 
sage -t  "devel/sage-combinat/sage/groups/perm_gps/permgroup.py"
**********************************************************************
File "/opt/sage/devel/sage-combinat/sage/groups/perm_gps/permgroup.py", line 2550:
    sage: G.isomorphism_to(H)
Expected:
    Permutation group morphism:
      From: Permutation Group with generators [(2,3), (1,2,3)]
      To:   Permutation Group with generators [(1,2,4), (1,4)]
      Defn: [(2,3), (1,2,3)] -> [(2,4), (1,2,4)]
Got:
    Permutation group morphism:
      From: Permutation Group with generators [(2,3), (1,2,3)]
      To:   Permutation Group with generators [(1,2,4), (1,4)]
      Defn: [(2,3), (1,2,3)] -> [(2,4), (1,4,2)]
**********************************************************************
1 items had failures:
   1 of   9 in __main__.example_73

Is this a random test ? Is this test really unrandonized (sorry for such a word) ? I am running a 11.4 Ubuntu up to date on a macbook santa rosa 4,1. I got this faillure both using the sage combinat queue and a separate branch with only 10334 and 10335.

Also, in sage/groups/perm-gps/permgroup.py line 1119, there is still the micro typo : postition --> position

Anyway, It is a very very nice work and improvement of permutation groups.

comment:19 Changed 8 years ago by mhansen

The failure you see is at #11427 . I'll put up a new patch fixing the typo.

Changed 8 years ago by mhansen

comment:20 Changed 8 years ago by nborie

  • Reviewers changed from Robert Miller to Robert Miller, Rob Beezer, Nicolas Borie
  • Status changed from needs_review to positive_review

Nice shoot! I confirm that I use a lot the Gap database especially for Transitive Groups. The optional package was installed on my machine.

Thanks for your work and your rapidity for fixing this last typo.

I give this patch a positive review.

comment:21 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-5.0 to sage-4.7.1

comment:22 follow-up: Changed 8 years ago by nthiery

  • Description modified (diff)

For the record: when applying this patch with the sage-combinat queue on sage-4.7.2, I got an import loop, and used the extra patch to fix it.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 8 years ago by mhansen

Replying to nthiery:

For the record: when applying this patch with the sage-combinat queue on sage-4.7.2, I got an import loop, and used the extra patch to fix it.

On 4.7.2?

comment:24 in reply to: ↑ 23 Changed 8 years ago by nthiery

Replying to mhansen:

Replying to nthiery:

For the record: when applying this patch with the sage-combinat queue on sage-4.7.2, I got an import loop, and used the extra patch to fix it.

On 4.7.2?

Oops, 4.7.1 alpha2!

Sage-Combinat is not that much in advance :-)

comment:25 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:26 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

New patch needs review.

comment:27 in reply to: ↑ 26 Changed 8 years ago by rbeezer

  • Keywords sd31 added
  • Status changed from needs_review to needs_work

Replying to jdemeyer:

New patch needs review.

Almost passes all tests on 4.7.1.alpha2 and I get no import problems.

But the German documentation has old _repr_ behavior. Aargh.

sage -t  -long -force_lib ooks/devel/sage/doc/de/tutorial/tour_groups.rst # 1 doctests failed
sage -t  -long -force_lib ooks/devel/sage/doc/de/tutorial/interfaces.rst # 1 doctests failed

Patch coming up, maybe.

Changed 8 years ago by rbeezer

comment:28 follow-up: Changed 8 years ago by rbeezer

  • Status changed from needs_work to needs_review

German doc patch should solve above problems. Gotta run - ticket needs "apply" block....

comment:29 Changed 8 years ago by rbeezer

  • Description modified (diff)

comment:30 follow-up: Changed 8 years ago by jdemeyer

Note that trac_10335-permgroup_domain-import_loop_fix-nt.patch should have its commit message changed.

comment:31 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

Changed 8 years ago by nthiery

Merge only if needed

comment:32 in reply to: ↑ 30 Changed 8 years ago by nthiery

Replying to jdemeyer:

Note that trac_10335-permgroup_domain-import_loop_fix-nt.patch should have its commit message changed.

Oops, done.

comment:33 in reply to: ↑ 28 Changed 8 years ago by nthiery

  • Status changed from needs_review to positive_review

Replying to rbeezer:

German doc patch should solve above problems. Gotta run - ticket needs "apply" block....

Sounds good to me assuming the patch bot is happy.

comment:34 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The Russian tutorial (#9378) also needs to be fixed:

sage -t  -force_lib devel/sage/doc/ru/tutorial/interfaces.rst
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha0/devel/sage-main/doc/ru/tutorial/interfaces.rst", line 167:
    sage: G.center()
Expected:
    Permutation Group with generators [()]
Got:
    Subgroup of (Permutation Group with generators [(3,4), (1,2,3)(4,5)]) generated by [()]
**********************************************************************
sage -t  -force_lib devel/sage/doc/ru/tutorial/tour_groups.rst
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha0/devel/sage-main/doc/ru/tutorial/tour_groups.rst", line 24:
    sage: G.center()
Expected:
    Permutation Group with generators [()]
Got:
    Subgroup of (Permutation Group with generators [(3,4), (1,2,3)(4,5)]) generated by [()]
**********************************************************************

Changed 8 years ago by rbeezer

German and Russian language fixes combined

comment:35 in reply to: ↑ 34 ; follow-up: Changed 8 years ago by rbeezer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

The Russian tutorial (#9378) also needs to be fixed:

Applied #9378 and the two main patches here. (I do not understand what "merge if needed" is suppose to mean - so I included that). Rolled the Russian fixes up with the German fixes into one patch.

Passes all tests in doc directory.

Could we please review and merge this before anymore new languages surface? ;-) ;-)

Rob

comment:36 in reply to: ↑ 35 Changed 8 years ago by nthiery

  • Reviewers changed from Robert Miller, Rob Beezer, Nicolas Borie to Robert Miller, Rob Beezer, Nicolas Borie, Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

Replying to rbeezer:

Applied #9378 and the two main patches here. (I do not understand what "merge if needed" is suppose to mean - so I included that).

See comment 23 above.

Rolled the Russian fixes up with the German fixes into one patch.

Passes all tests in doc directory.

Could we please review and merge this before anymore new languages surface? ;-) ;-)

I checked your new patch, and am happy with it. Back to positive review. Thanks!

comment:37 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 8 years ago by jdemeyer

  • Merged in sage-4.7.2.alpha0 deleted
  • Resolution fixed deleted
  • Status changed from closed to new
  • Work issues set to Solaris doctest error

Unfortunately, there is a doctest error on Solaris (to be more precise, the buildbot machine mark2):

sage -t -long  -force_lib devel/sage/sage/groups/perm_gps/permgroup.py
**********************************************************************
File "/home/buildbot/build/sage/mark-1/mark_full/build/sage-4.7.2.alpha0/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 811:
    sage: G.gens_small()
Expected:
    [('a','b'), ('a','b','c')]
Got:
    [('b','c'), ('a','b','c')]
**********************************************************************

comment:39 Changed 8 years ago by mhansen

What's the best way to test a fix on that machine? Do I have to log into mark2 and do a full build or is there a way to access that build?

comment:40 Changed 8 years ago by mhansen

  • Cc jdemeyer added

Ping.

comment:41 Changed 8 years ago by mhansen

jdemeyer:

What's the best way to test a fix on that machine? Do I have to log into mark2 and do a full build or is there a way to access that build?

Changed 8 years ago by davidloeffler

apply over previous patches

comment:42 Changed 8 years ago by davidloeffler

  • Description modified (diff)
  • Status changed from new to needs_review

I don't have access to a Solaris machine to test this on; but, looking at the code, it's clear that (a) this Sage function just wraps the corresponding one in Gap, (b) the answer returned on Solaris is just as much a valid answer as that returned on other systems, and (c) there is already a perfectly good doctest for that function.

I strongly suggest that we change this doctest to something less sensitive, given that there is a huge pile of other code waiting on this. I have just uploaded a patch which does this.

Changed 8 years ago by davidloeffler

apply over previous patches

comment:43 Changed 8 years ago by davidloeffler

  • Description modified (diff)

This has now bitrotted: several doctests fail in trivial ways on sage 4.7.2.alpha2. I've just uploaded a patch that fixes these.

comment:44 Changed 8 years ago by davidloeffler

  • Work issues Solaris doctest error deleted

I've tested this on the Sun machine "mark" on skynet, and all doctests pass (except for a pre-existing error in matrix_double_dense which has nothing to do with this patch. Can someone please visually check that the two most recent patches trac_10335-solaris_issue.patch and trac_10335-updates.patch are morally sound? I'd be really disappointed if this bitrotted yet again.

comment:45 Changed 8 years ago by davidloeffler

FWIW, the "pre-existing error in matrix_double_dense" is a known issue coming from #10795, which has apparently been de-merged from the next release for this reason.

comment:46 Changed 8 years ago by rbeezer

  • Authors changed from Mike Hansen to Mike Hansen, David Loeffler
  • Status changed from needs_review to positive_review

David's two recent patches look good to me, and update from a point where this had a positive review.

Applies, builds, passes long tests on 4.7.2.alpha2 and 64-bit Ubuntu 11.04. So, back to positive review with a hope it merges it OK, finally.

comment:47 follow-up: Changed 8 years ago by leif

I'm not sure whether removing a doctest is the proper way to fix a doctest error... ;-)

comment:48 in reply to: ↑ 47 Changed 8 years ago by rbeezer

Replying to leif:

I'm not sure whether removing a doctest is the proper way to fix a doctest error... ;-)

I agree. But in this case, I agree with David's argument. This patch implements a highly desirable feature with many doctests. And if I understand right, GAP uses some probabilistic algorithms, so we could tie ourselves in knots trying to rewrite this doctest.

My understanding (and some grep'ing appears to confirm) is that this is a new test. Would the patch have gone in without it if it had never appeared? I think so. If an existing test were failing, I'd be a lot less cavalier about dropping it.

Anyway, my $0.02 worth.

comment:49 follow-up: Changed 8 years ago by mhansen

I think the correct fix would to be to add one of the current_randstate().set_seed_gap() lines to the beginning of that function.

comment:50 in reply to: ↑ 49 Changed 8 years ago by leif

Replying to mhansen:

I think the correct fix would to be to add one of the current_randstate().set_seed_gap() lines to the beginning of that function.

Or otherwise at least tag the doctest with # random, # not tested, # platform-specific(?) or whatever, since doing so documents the different behaviour.

comment:51 Changed 8 years ago by davidloeffler

I agree that in an ideal world it would be better to have another doctest for this function, but as Rob points out, there was no such doctest before this ticket anyway. So it seems absurd to hold up all of the other new functionality added by this ticket, and the two other positively-reviewed tickets depending on it, for such a reason.

If it matters that much, perhaps we should have a new ticket "improve doctests for wrapper of Gap function xxx".

comment:52 Changed 8 years ago by jdemeyer

I agree with leif that adding a simple "# random" is an easy solution which at least documents the problem.

comment:53 follow-ups: Changed 8 years ago by davidloeffler

Aaargh! Do what you like, just *MERGE IT SOMEHOW*! If we miss the boat with this one again, and consequently #11422, #11598, #5048, #11601 and #10546 end up still trapped in purgatory, I will have to think very seriously about whether working on Sage in future is a productive use of my time.

comment:54 in reply to: ↑ 53 ; follow-up: Changed 8 years ago by leif

Replying to davidloeffler:

Aaargh! Do what you like, just *MERGE IT SOMEHOW*! If we miss the boat with this one again, and consequently #11422, #11598, #5048, #11601 and #10546 end up still trapped in purgatory, I will have to think very seriously about whether working on Sage in future is a productive use of my time.

Well, to be honest, it's not my problem that this ticket bit-rottened. I started merging tickets "officially" into alpha3 about two weeks ago IIRC (including some you participated in); this ticket got positive review 20 hours ago.

I agree that it's a pity, not just because other already positively reviewed tickets -- three of the other ones you mentioned still need review -- depend on this one, but as I told Rob yesterday, it would have to get rebased anyway, because of (at least) #11749, and I'm not going to back the latter out (or try to reapply it on top of tickets that haven't had positive review recently, again having to manually resolve conflicts and errors, some showing up only later during testing).

We haven't yet talked about the release plans for the final Sage 4.7.2, but I think there'll still be an alpha4 before we go into feature freeze, as there are other important things that still have to get fixed and should be solved in a final.

So IMHO it would be best to wait for the alpha3, hopefully released within the next days, and then rebase this ticket (and if necessary the others) on that.

[I didn't touch the tickets having milestone "sage-pending" (including #11422 and #11598) at all, since I don't know why Jeroen put them there, although I have to admit I also didn't try to find out, mainly because of lack of time. I early did ping the authors and reviewers of all other tickets that were holding up other positively reviewed ones though, with some success.]


W.r.t. the doctest with varying output:

It would IMHO be best to give the alternative, also correct result as an example, too, marking both # not tested, with an appropriate comment.

(And perhaps add a real doctest which tests a property common to both.)

comment:55 in reply to: ↑ 54 Changed 8 years ago by jdemeyer

Replying to leif:

I didn't touch the tickets having milestone "sage-pending" (including #11422 and #11598) at all, since I don't know why Jeroen put them there

I put them there because of the dependency on this ticket, no other reason.

comment:56 in reply to: ↑ 53 ; follow-up: Changed 8 years ago by rbeezer

Replying to davidloeffler:

Aaargh! Do what you like, just *MERGE IT SOMEHOW*!

Leif - thanks for the help with this one. With classes starting, I really wasn't in a position to be chasing the evolving alpha, so I had to pass on that offer, but appreciate the willingness.

David - How about the following?

  1. Wait for alpha3 (which sounds big already).
  1. I mark the disputed test # random.
  1. I rebase on alpha3 in preparation for alpha4.
  1. You review (2) and (3).
  1. We quickly identify if the tickets in Permutation Group Purgatory needing changes/rebasing.

Lemme know and I'll carve out time to make it happen.

Rob

comment:57 follow-up: Changed 8 years ago by davidloeffler

That sounds like a good solution -- thanks for volunteering to do (2) and (3). I can take care of (5).

My apologies, particularly to Leif, for my outburst earlier.

comment:58 in reply to: ↑ 57 Changed 8 years ago by rbeezer

  • Status changed from positive_review to needs_work

Replying to davidloeffler:

That sounds like a good solution -- thanks for volunteering to do (2) and (3). I can take care of (5).

Great! We have a plan. Thanks for taking (5).

Leif and/or Jeroen - feel free to ping us if we need to move quickly on something.

At the ready, Rob

comment:59 in reply to: ↑ 56 ; follow-up: Changed 8 years ago by leif

Replying to rbeezer:

  1. Wait for alpha3 (which sounds big already).

[...]

  1. I rebase on alpha3 in preparation for alpha4.

[...]

  1. We quickly identify if the tickets in Permutation Group Purgatory needing changes / rebasing.


For the impatient, an updated tarball and source tree for upgrading can be found in

http://sage.math.washington.edu/home/leif/Sage/release/sage-4.7.2.alpha3-prerelease/.

I don't expect further changes to the alpha3 release, except for a few only fixing some doctest errors due to numerical noise below

  • sage/rings/polynomial/ and
  • sage/matrix/,

and perhaps some clean-up like adding ticket numbers to commit messages lacking them, so it should be safe to (re)base patches on this version.

comment:60 in reply to: ↑ 59 Changed 8 years ago by rbeezer

Replying to leif:

For the impatient

Perfect - thanks! Building now, will tinker tonight.

Changed 8 years ago by davidloeffler

patch against 4.7.2.alpha3 prerelease

comment:61 follow-up: Changed 8 years ago by davidloeffler

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I had a few minutes free this afternoon, so I've done a rebased patch myself. This is just the existing patches rebased and folded, but with the doctest in permgroup.py marked as random rather than deleted, and with examples of the output I got on a Linux and a Solaris machine.

Rob: any chance you could review this? Apologies if you'd already started rebasing this yourself as per our earlier agreement -- in that case feel free to ignore my patch and I'll review yours.

Leif: you might be interested to hear that this prerelease builds and passes long doctests without problems on my 64-bit Linux machine -- no numerical noise or anything like that.

comment:62 follow-up: Changed 8 years ago by rbeezer

David - no problem, I'll do a review. I have no free minutes today. So tonight. ;-)

Probably about 8-10 hours from now. Thanks for hopping on this one.

comment:63 in reply to: ↑ 61 Changed 8 years ago by leif

Replying to davidloeffler:

Leif: you might be interested to hear that this prerelease builds and passes long doctests without problems on my 64-bit Linux machine -- no numerical noise or anything like that.

If you also tell me what CPU you built on... (e.g. top of /proc/cpuinfo, i.e., only for the first core...)

Might be interesting.

comment:64 in reply to: ↑ 62 Changed 8 years ago by rbeezer

Replying to rbeezer:

Probably about 8-10 hours from now.

My build was interrupted (long story) and now two files are giving segfaults when I doctest, with or without the patch. Don't panic - likely entirely all my fault. So anyway, I'm starting over and building overnight. Stay tuned.

Rob

comment:65 follow-up: Changed 8 years ago by rbeezer

alpha3-prerelease segfaults during testing, without patch. Errors, followed by system info. I can follow up with more testing, if requested.

The following tests failed:

        sage -t  -long -force_lib devel/sage/sage/modular/modsym/ambient.py # 0 doctests failed
        sage -t  -long -force_lib devel/sage/sage/modular/hecke/ambient_module.py # 0 doctests failed
        sage -t  -long -force_lib devel/sage/sage/modular/hecke/hecke_operator.py # 0 doctests failed
----------------------------------------------------------------------
Total time for all tests: 2701.6 seconds
make: *** [ptestlong] Error 128
rob@lava:/sage/sage-4.7.2.alpha3-prerelease$ ./sage -t  -long -force_lib devel/sage/sage/modular/modsym/ambient.py
sage -t -long -force_lib "devel/sage/sage/modular/modsym/ambient.py"
/sage/sage-4.7.2.alpha3-prerelease/local/lib/libcsage.so(print_backtrace+0x31)[0x7fd8e335834f]
/sage/sage-4.7.2.alpha3-prerelease/local/lib/libcsage.so(sigdie+0x14)[0x7fd8e3358381]
/sage/sage-4.7.2.alpha3-prerelease/local/lib/libcsage.so(sage_signal_handler+0x20e)[0x7fd8e3357fb4]
/lib/x86_64-linux-gnu/libpthread.so.0(+0xfc60)[0x7fd8e484fc60]
/sage/sage-4.7.2.alpha3-prerelease/local/lib/libatlas.so(ATL_dJIK44x44x44TN44x44x0_a1_b0+0x9d)[0x7fd8da12c3ed]

------------------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off(). You might
want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate.
------------------------------------------------------------------------
Segmentation fault

         [5.9 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t -long -force_lib "devel/sage/sage/modular/modsym/ambient.py"
Total time for all tests: 5.9 seconds
rob@lava:/sage/sage-4.7.2.alpha3-prerelease$ uname -a
Linux lava 2.6.38-10-generic #46-Ubuntu SMP Tue Jun 28 15:07:17 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux
rob@lava:/sage/sage-4.7.2.alpha3-prerelease$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/x86_64-linux-gnu/gcc/x86_64-linux-gnu/4.5.2/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 4.5.2-8ubuntu4' --with-bugurl=file:///usr/share/doc/gcc-4.5/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.5 --enable-shared --enable-multiarch --with-multiarch-defaults=x86_64-linux-gnu --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib/x86_64-linux-gnu --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.5 --libdir=/usr/lib/x86_64-linux-gnu --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-plugin --enable-gold --enable-ld=default --with-plugin-ld=ld.gold --enable-objc-gc --disable-werror --with-arch-32=i686 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-8ubuntu4) 
rob@lava:/sage/sage-4.7.2.alpha3-prerelease$ cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Core(TM) i7-2600 CPU @ 3.40GHz
stepping        : 7
cpu MHz         : 1600.000
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 0
cpu cores       : 4
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 x2apic popcnt aes xsave avx lahf_lm ida arat epb xsaveopt pln pts dts tpr_shadow vnmi flexpriority ept vpid
bogomips        : 6822.05
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

comment:66 in reply to: ↑ 65 ; follow-up: Changed 8 years ago by leif

Replying to rbeezer:

alpha3-prerelease segfaults during testing, without patch. Errors, followed by system info. I can follow up with more testing, if requested.

That's either due to Linaro, or, more likely, an ATLAS problem, since our version afaik doesn't yet support Sandy Bridge (second generation "Core i").

You could try using your system's ATLAS (may have to install it with your package manager first), by doing:

$ export SAGE_ATLAS_LIB=/usr/lib64/atlas # the directory; usual place on 64-bit Linux
$ rm $SAGE_ROOT/spkg/installed/atlas-*
$ cd $SAGE_ROOT
$ env SAGE_UPGRADING=yes make # or "... make build"

The SAGE_UPGRADING=yes is important to (really) rebuild all dependent packages as well.

comment:67 in reply to: ↑ 66 Changed 8 years ago by leif

Replying to leif:

$ export SAGE_ATLAS_LIB=/usr/lib64/atlas # the directory; usual place on 64-bit Linux
...

Might be /usr/lib/x86_64-linux-gnu/atlas/ on your system; you should check this before attempting to build... ;-)

comment:68 follow-up: Changed 8 years ago by rbeezer

Thanks, Leif. Ubuntu installs ATLAS into /usr/lib64/atlas-base. Then setting up everything as you describe, suitably adjusted yields:

Unable to find one of liblapack, libcblas, libatlas or libf77blas
in the directory /usr/lib64/atlas-base
Set SAGE_ATLAS_LIB to the directory containing liblapack, libcblas,
libatlas and libf77blas (either .a or .so extensions) if you wish to
use existing ATLAS libraries. For more details, see

I've installed the packages liblapack3gf, libblas3gf but cannot see anything to provide libf77blas and do not know if I am getting libcblas. Furthermore, these all go to different directories.

Any advice? My Sandy Bridge motherboard is about 8 months old. Will anybody with a newer chip have to go through a drill like this? Would Volker's ATLAS ticket be a better/workable approach?

comment:69 in reply to: ↑ 68 Changed 8 years ago by leif

Replying to rbeezer:

I've installed the packages liblapack3gf, libblas3gf but cannot see anything to provide libf77blas and do not know if I am getting libcblas. Furthermore, these all go to different directories.

I've asked Volker on the IRC for help. (Perhaps drop in if you can.)

If some libraries are simply in different directories, you could just create symbolic links to them in the .../atlas-base/ directory. (Although there IMHO shouldTM be a directory containing all; are there some more self-contained ATLAS packages you could install?)


My Sandy Bridge motherboard is about 8 months old.

That shouldn't matter; Intel's bug just causes a SATA part of the chipset to die earlier.

Whether AVX is available depends on your board (and chipset); if you have on-board graphics, AVX isn't usable.


Will anybody with a newer chip have to go through a drill like this?

Simon King did successfully build the prerelease on a (Sandy Bridge) Core i3 with Debian x86_64; there only five doctests (in three files) failed due to numerical noise (the same doctests that failed on some other "rare" and older platforms).

comment:70 follow-up: Changed 8 years ago by vbraun

The following bug in Ubuntu launchpad says that the symlinks for the standard libraries are not created: https://bugs.launchpad.net/ubuntu/+source/atlas/+bug/769180 So you either create them manually (in a directory of your choice), or you compile ATLAS again. For the latter, you should probably try to set

export SAGE_ATLAS_ARCH=Corei7,SSE3,SSE2,SSE1

This works only with the new atlas spkg (4.7.2.alpha3+)

comment:71 follow-up: Changed 8 years ago by vbraun

AVX is just the CPU instruction set extension du jour. The version of ATLAS that we are shipping knows nothing about AVX. All 2011 sandy bridge cpus have it, irrespective of on-board graphics.

comment:72 in reply to: ↑ 71 Changed 8 years ago by leif

Replying to vbraun:

AVX is just the CPU instruction set extension du jour. The version of ATLAS that we are shipping knows nothing about AVX.

Aha. Wasn't sure about that. But does 3.8.4 "know" Sandy Brigde already?


All 2011 sandy bridge cpus have it, irrespective of on-board graphics.

They have it, but you can only use it when the CPU doesn't also act as a GPU. And afaik that's hard-wired, i.e., you even cannot use it if your motherboard has on-board graphics you don't actually use.

comment:73 in reply to: ↑ 70 Changed 8 years ago by leif

Replying to vbraun:

This works only with the new atlas spkg (4.7.2.alpha3+)

I guess you meant alpha2+.

Apparently alpha2 in contrast did not segfault for Rob (on that machine), despite having the same ATLAS spkg. At least that's my understanding.

comment:74 Changed 8 years ago by vbraun

Oh yes, it was merged in alpha2.

This is now completely OT, but my (Sandy Bridge) Thinkpad W520 uses the on-board GPU and has AVX:

[vbraun@volker-laptop-two ~]$ cat /proc/cpuinfo | head -24
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Core(TM) i7-2720QM CPU @ 2.20GHz
stepping        : 7
cpu MHz         : 2201.000
cache size      : 6144 KB
physical id     : 0
siblings        : 8
core id         : 0
cpu cores       : 4
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx rdtscp lm constant_tsc arch_perfmon pebs bts nopl xtopology nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 x2apic popcnt aes xsave avx lahf_lm ida arat epb xsaveopt pln pts dts tpr_shadow vnmi flexpriority ept vpid
bogomips        : 4385.92
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

comment:75 follow-up: Changed 8 years ago by rbeezer

Leif and Volker,

Thanks for the quick help, and I apologize to the group theorists for diverting this ticket. I've got to attend to some other things the rest of today, but I'll see if I can get the symlinks in place with guidance from that bug report.

I have Sandy Bridge with no on-board graphics. I have built and tested alpha2 - this is my first problem like this.

Rob

comment:76 in reply to: ↑ 75 Changed 8 years ago by leif

Replying to rbeezer:

Leif and Volker,

Thanks for the quick help, and I apologize to the group theorists for diverting this ticket.

Yep, we can continue the discussion on e.g. this sage-release thread, or -- preferably -- a new one there.

comment:77 Changed 8 years ago by rbeezer

  • Authors changed from Mike Hansen, David Loeffler to Mike Hansen, Jason Hill, David Loeffler
  • Status changed from needs_review to positive_review

alpha3 prerelease build is fixed and passes long tests, with and without this rebased patch. To quote William "merge that sucker"! In other words, positive review.

Second line of patch summary message credits Jason Hill - I'll add him to the authors, and Mike's name is still on the main patch.

David - feel free to cc me on any of the follow-on tickets that might need review, especially if they are cosmetic changes. I have not yet looked at them at all.

Leif - thanks for your attention to this one and help with my build challenges.

Everybody else - my apologies again for getting this ticket off the tracks. I need to post a summary on sage-release.

Rob

comment:78 Changed 8 years ago by davidloeffler

Hi Rob,

Thanks for the review! I have tested the two tickets that follow on directly from this (#11422 and #11598) and they apply fine and pass doctests, so no futher reviewing is needed; I'm going to move them back to the 4.7.2 milestone (from "sage-pending").

Regards, David

comment:79 Changed 8 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:80 Changed 8 years ago by nborie

Hello and sorry to add a comment on a closed ticket. I have a patch which depend on it and the patchbot try to apply 2 patches instead one here.

apply trac_10335-permgroup_domain-rebase.patch

Note: See TracTickets for help on using tickets.