Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14291 closed enhancement (fixed)

Orbits of tuples and sets

Reported by: ncohen Owned by: joyner
Priority: major Milestone: sage-5.10
Component: group theory Keywords:
Cc: dimpase Merged in: sage-5.10.beta0
Authors: Nathann Cohen, Volker Braun Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

A small patch to let us compute the orbits of sets and tuples by exposing it from GAP.

Apply :

  1. trac_14291-v2.patch
  2. trac_14291_reviewer.patch

Attachments (7)

trac_14291.patch (5.1 KB) - added by ncohen 6 years ago.
14291_reviewer.patch (5.6 KB) - added by dimpase 6 years ago.
trac_14291-rev2.patch (4.0 KB) - added by ncohen 6 years ago.
ducktype_design.patch (4.3 KB) - added by dimpase 6 years ago.
trac_14291-v2.patch (8.3 KB) - added by ncohen 6 years ago.
trac_14291_reviewer.2.patch (7.2 KB) - added by vbraun 6 years ago.
Updated patch
trac_14291_reviewer.patch (8.4 KB) - added by vbraun 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (63)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

There is a small problem with cached functions right now... -_-

Nathann

comment:2 Changed 6 years ago by dimpase

this looks weird:

def orbit(self, point, onsets = False, ontuples = False): 

What about

def orbit(self, point, action = "onpoints"): 

(or "OnPoints" ?) which is the default, and then sort out the cases action = "onsets", etc. ?

comment:3 Changed 6 years ago by ncohen

Totally right...

Just updated it ! And the function only takes tuples as arguments now. I so hate it when design decisions have to be changed for technical issues -_-.

Nathann

Changed 6 years ago by ncohen

comment:4 follow-up: Changed 6 years ago by dimpase

There are more useful permutation group actions, see http://www.gap-system.org/Manuals/doc/ref/chap41.html

Namely, OnPairs, OnSetsSets, OnSetsDisjointSets, OnSetsTuples, OnTuplesSets, OnTuplesTuples all should work just fine.

One has to canonize the representative to start with, for some of them, see here. E.g.

Orbit(SymmetricGroup(5),Set([[2,4],[1,3]]),OnSetsSets);

is OK, but

Orbit(SymmetricGroup(5),[[2,4],[1,3]],OnSetsSets);

gives an error.

Less sure about OnIndeterminates.

There is also a support for user-defined actions, but this would mean either specifying a piece of GAP code, or providing a callback, which seems to be too crazy to implement.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by ncohen

Helloooooooo !!

There are more useful permutation group actions, see http://www.gap-system.org/Manuals/doc/ref/chap41.html

Yepyep, I saw them but I really only need OnTuples and OnSets myself, and as I do not really see how I could use the others I do not think that it would be wise to expose them myself. Soooo unless you want to write that additional patch yourslf, ... :-)

Nathann

Last edited 6 years ago by ncohen (previous) (diff)

comment:6 Changed 6 years ago by dimpase

  • Description modified (diff)

comment:7 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Helloooooooo !!

There are more useful permutation group actions, see http://www.gap-system.org/Manuals/doc/ref/chap41.html

Yepyep, I saw them but I really only need OnTuples and OnSets myself, and as I do not really see how I could use the others I do not think that it would be wise to expose them myself. Soooo unless you want to write that additional patch yourslf, ... :-)

OK, needs your review now :-P

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by ncohen

  • Status changed from needs_review to needs_work

OK, needs your review now :-P

Cool :-P

Well, for a start sage -coverage will not like the documentation of supported_actions at all. Why do you want to create a function just to list them ? Why don't you just include in the methods, or make it a variable in the module ? (anyway you still have to split cases in orbits in order to know what to return).

Would it be possible to add a link toward GAP's documentation where all those actions are defined ?

Nathann

Changed 6 years ago by dimpase

comment:9 in reply to: ↑ 8 Changed 6 years ago by dimpase

Replying to ncohen:

OK, needs your review now :-P

Cool :-P

Well, for a start sage -coverage will not like the documentation of supported_actions at all. Why do you want to create a function just to list them ? Why don't you just include in the methods, or make it a variable in the module ? (anyway you still have to split cases in orbits in order to know what to return).

made it a variable...

Would it be possible to add a link toward GAP's documentation where all those actions are defined ?

added...

Changed 6 years ago by ncohen

comment:10 follow-up: Changed 6 years ago by ncohen

  • Description modified (diff)

Some small modifications to the documentation. Besides this :

1) What would you think of making supported_actions a variable of the module itself ? And what of making it a hidden variable ?

2) What would you think of sorting the input when it represents a set ?.. GAP's error message when it is fed with an unsorted list is not that clear :

Error, OnSets: <set> must be a set (not a list (plain,cyc,nsort,imm))

I will do that if you agree with it.

Nathann

comment:11 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Some small modifications to the documentation. Besides this :

1) What would you think of making supported_actions a variable of the module itself ? And what of making it a hidden variable ?

The current setup allows it to be extended by the user. One can install via gap.eval() a GAP function, say, "OnBlah", and then use it. You can't do this with a module variable.

2) What would you think of sorting the input when it represents a set ?.. GAP's error message when it is fed with an unsorted list is not that clear :

Error, OnSets: <set> must be a set (not a list (plain,cyc,nsort,imm))

I will do that if you agree with it.

sure this can be done as you prefer. However, there is a more serious issue with self.action:

sage: G = PermutationGroup([ [('c','d')], [('a','c')] ])
sage: xx=G.orbit(('a','c'),"OnPairs")
sage: G.action(xx,"OnPairs")
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-4-540be1338037> in <module>()
----> 1 G.action(xx,"OnPairs")
...

Indeed, _domain_to_gap[x] needs to be applied onto such orbits. I am thinking that one should have recursive function version of _domain_to_gap[x] and of _domain_from_gap[x] to deal with things like sets, sets of sets, etc in one go rather than call them explicitly.

One can also (or instead) attach such a pair of functions to each action in supported_actions to do this rewriting.

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

comment:12 in reply to: ↑ 11 Changed 6 years ago by ncohen

The current setup allows it to be extended by the user. One can install via gap.eval() a GAP function, say, "OnBlah", and then use it. You can't do this with a module variable.

Well, if a action is being used that is not already in the source code, then there is no way for Sage to guess how to translate the result before returning it, is there ?

Otherwise we could just get rid of this list, cross our fingers when GAP is fed with something we ha not thought of, as it would raise an Exception in the worst case ?..

sure this can be done as you prefer. However, there is a more serious issue with self.action:

Arg. Right :-/

Indeed, _domain_to_gap[x] needs to be applied onto such orbits. I am thinking that one should have recursive function version of _domain_to_gap[x] and of _domain_from_gap[x] to deal with things like sets, sets of sets, etc in one go rather than call them explicitly.

One can also (or instead) attach such a pair of functions to each action in supported_actions to do this rewriting.

Hmmmm... I don't like it very much to be honest... In these situations I think that a bit of code duplication is better than a weird generic way to handle it.

Nathann

comment:13 Changed 6 years ago by dimpase

I just added a new patch (to be applied on top of the rest) that solves these issues. Please have a look if you like it. (It still needs to be cleaned a bit, and more tests ought to be added). It doesn't use the supported_actions list.

comment:14 follow-up: Changed 6 years ago by ncohen

HMmmmmmmmmm :-/

I'm sorry to say this because I like that code, but I just fixed doctests in homology/simplicial_complex where an automorphism group is defined over the elements of a simplicial complex... And I am afraid that you will find both 'a', 'b', and ['a','b'] as elements of the domain :-/

Nathann

comment:15 in reply to: ↑ 14 Changed 6 years ago by dimpase

Replying to ncohen:

HMmmmmmmmmm :-/

I'm sorry to say this because I like that code, but I just fixed doctests in homology/simplicial_complex where an automorphism group is defined over the elements of a simplicial complex... And I am afraid that you will find both 'a', 'b', and ['a','b'] as elements of the domain :-/

Well, this is trivial to fix, just swap the actions in try/except blocks in dom_to_gap internal functions.

By the way, why would one keep several actions in case of simplicial complexes, on letters and on pairs, etc? This is akin to keeping actions on vertices and edges in case of automorphisms of (simple) graphs, right? Was that a poor design in the first place? Besides, this seems to be inefficient to keep permutations in dictionaries.

Nathann

Changed 6 years ago by dimpase

comment:16 Changed 6 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, updated as I mentioned in my previous comment, with more examples added.

comment:17 follow-up: Changed 6 years ago by ncohen

Helloooooooooooooo !!!

Well, this does not solve the problem either Dima. In case of doubt between an element (a,b,c) and the list of elements (a,b,c) it will return the ELEMENT (a,b,c), but it it computes the orbit of a which happens to be (a,b,c) then returning an ELEMENT (a,b,c) instead of the list (a,b,c) is a mistake too :-/

Nathann

comment:18 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Helloooooooooooooo !!!

Well, this does not solve the problem either Dima. In case of doubt between an element (a,b,c) and the list of elements (a,b,c) it will return the ELEMENT (a,b,c), but it it computes the orbit of a which happens to be (a,b,c) then returning an ELEMENT (a,b,c) instead of the list (a,b,c) is a mistake too :-/

This does not seem to present a problem, as the action of the group is the same (trivial).

But the real problem is with tuples vs sets. E.g. if your domain has tuples, it will not be possible to compute the action on subsets.

Can't you typify the domain elements somehow, so that they are recognised as atoms without this kind of problem? (Or mangle them by some other means...)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 6 years ago by ncohen

Can't you typify the domain elements somehow, so that they are recognised as atoms without this kind of problem? (Or mangle them by some other means...)

Hmmmmmmmmm.... I have no idea how, an I would hate to have to use the Parent/Element? stuff to do that :-P

Well, I guess the best is to do as you first did and to split according to the value of action.

By the way, I noticed in the code source of Graph.is_edge_transitive a trick we could use there :

"OrbitLength("+str(A._gap_())+",Set(" + str(e) + "),OnSets);"

If we wrap the lists with a Set, then GAP itself reorders them and there is nothing to fear when giving sets at Sage level :-)

Nathann

comment:20 in reply to: ↑ 19 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Can't you typify the domain elements somehow, so that they are recognised as atoms without this kind of problem? (Or mangle them by some other means...)

Hmmmmmmmmm.... I have no idea how, an I would hate to have to use the Parent/Element? stuff to do that :-P

Well, I guess the best is to do as you first did and to split according to the value of action.

this is too ugly, uglier than using a class, or whatever. Actually, I think it is simply wrong to let groups act on some totally unstructured arbitrary sh*t, and this is exactly what happens right now with allowing arbitrary domain elements, for which any relation to the original domain is lost.

Say, if the domain of a group is {1,2,3,4,{1,2},{2,3}}, the very notion of the orbit of {1,2} is becoming ambiguous, as in a sane world {1,2} is a subset of {1,2,3,4}, and not a random label.

Dima

comment:21 in reply to: ↑ 20 ; follow-up: Changed 6 years ago by ncohen

Yoooooooooooo !

this is too ugly, uglier than using a class, or whatever. Actually, I think it is simply wrong to let groups act on some totally unstructured arbitrary sh*t, and this is exactly what happens right now with allowing arbitrary domain elements, for which any relation to the original domain is lost.

Ahahaha. Deal. Do I remove all support for labels on edges and vertices from graphs ? I am sooooooooooo eager to :-P

Say, if the domain of a group is {1,2,3,4,{1,2},{2,3}}, the very notion of the orbit of {1,2} is becoming ambiguous, as in a sane world {1,2} is a subset of {1,2,3,4}, and not a random label.

Well, in Sage however it is not ambiguous :

g.orbit(Set([1,2]),action="OnPoints")
g.orbit(Set([1,2]),action="OnSets")

Nathann

comment:22 in reply to: ↑ 21 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Yoooooooooooo !

this is too ugly, uglier than using a class, or whatever. Actually, I think it is simply wrong to let groups act on some totally unstructured arbitrary sh*t, and this is exactly what happens right now with allowing arbitrary domain elements, for which any relation to the original domain is lost.

Ahahaha. Deal. Do I remove all support for labels on edges and vertices from graphs ? I am sooooooooooo eager to :-P

I don't think this is relevant. Here we have an example of a group acting on triples, and this is a kind of "primitive" action, all the other actions originate from it.

Anyhow, there is a hack which might fix all our sorrows: namely, add an extra level of () to the "most primitive" elements of the domain. I.e. if instead of {1,2,3,4,{1,2},{2,3}} there was a domain {{1},{2},{3},{4},{1,2},{2,3}} then the ambiguity does not arise, for {1,2} and {{1},{2}} are different things now.

Dima

Say, if the domain of a group is {1,2,3,4,{1,2},{2,3}}, the very notion of the orbit of {1,2} is becoming ambiguous, as in a sane world {1,2} is a subset of {1,2,3,4}, and not a random label.

Well, in Sage however it is not ambiguous :

g.orbit(Set([1,2]),action="OnPoints")
g.orbit(Set([1,2]),action="OnSets")

Nathann

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

I don't think this is relevant. Here we have an example of a group acting on triples, and this is a kind of "primitive" action, all the other actions originate from it.

Anyhow, there is a hack which might fix all our sorrows: namely, add an extra level of () to the "most primitive" elements of the domain. I.e. if instead of {1,2,3,4,{1,2},{2,3}} there was a domain {{1},{2},{3},{4},{1,2},{2,3}} then the ambiguity does not arise, for {1,2} and {{1},{2}} are different things now.

Come on Dima. Let's just rewrite the method as it was previously... If the action is on points we know what to do, if it is on something else we also know what to do, case by case.

It's not worth adding another layer just for that ...

Nathann

comment:24 in reply to: ↑ 23 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

I don't think this is relevant. Here we have an example of a group acting on triples, and this is a kind of "primitive" action, all the other actions originate from it.

Anyhow, there is a hack which might fix all our sorrows: namely, add an extra level of () to the "most primitive" elements of the domain. I.e. if instead of {1,2,3,4,{1,2},{2,3}} there was a domain {{1},{2},{3},{4},{1,2},{2,3}} then the ambiguity does not arise, for {1,2} and {{1},{2}} are different things now.

Come on Dima. Let's just rewrite the method as it was previously... If the action is on points we know what to do, if it is on something else we also know what to do, case by case.

It's not worth adding another layer just for that ...

it's for the general sanity of the setup. Let me post on sage-devel asking opinions, as I do think that this is a fundamental semantic problem of the design.

Needless to say, if I am declared insane there, then I would happily review whatever patch you would be willing to put here :)

Dima

Nathann

comment:25 in reply to: ↑ 24 ; follow-up: Changed 6 years ago by ncohen

Needless to say, if I am declared insane there, then I would happily review whatever patch you would be willing to put here :)

It is very kind of you but my patch for this ticket has been written for a while already. It has been delayed because you wanted to add more than I needed :-P

Nathann

comment:26 in reply to: ↑ 25 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

Needless to say, if I am declared insane there, then I would happily review whatever patch you would be willing to put here :)

It is very kind of you but my patch for this ticket has been written for a while already. It has been delayed because you wanted to add more than I needed :-P

The curiosity of the reviewer is sacred, don't you think so? :-)

Besides, it's easy to backport the patch here directly into the smallgraphs patch it came from, and put this ticket on hold.

Dima

Nathann

comment:27 in reply to: ↑ 26 Changed 6 years ago by ncohen

The curiosity of the reviewer is sacred, don't you think so? :-)

It is.

Besides, it's easy to backport the patch here directly into the smallgraphs patch it came from, and put this ticket on hold.

Oh, you are worried about #14283 ? I really don't need it, I write this kind of patches when I spent too much time thinking and cannot do anything useful anymore with my head. So I try to build a graph. But if it is not merged in the next ten months I do not mind at all. It is written somewhere, it is reviewed, my part has been done :-)

But still, I would love this patch to be available in Sage. And #14319 even more :-)

Nathann

Last edited 6 years ago by ncohen (previous) (diff)

comment:28 follow-up: Changed 6 years ago by vbraun

  1. @cached_method must not return lists, but only immutable things
    sage: S3.orbit(3)
    [3, 1, 2]
    sage: type(_)
    list
    
    Just return a tuple.
  1. It would be nice if orbit were smarter about the action type and make the output a set/tuple if necessary. For example, desirable behaviour would be
    sage: S3.orbit(3)       # in domain, use OnPoints
    (1, 2, 3)
    sage: S3.orbit((1,2))   # not in domain, a tuple, use OnTuples
    ((1, 2), (2, 3), (2, 1), (3, 1), (1, 3), (3, 2))
    sage: S3.orbit({1,2})   # not in domain, a set, use OnSets
    ({1, 2}, {2, 3}, {1, 3})
    

comment:29 in reply to: ↑ 28 ; follow-ups: Changed 6 years ago by dimpase

Replying to vbraun:

  1. @cached_method must not return lists, but only immutable things

OK, this should be easy.

  1. It would be nice if orbit were smarter about the action type and make the output a set/tuple if necessary.

sure, this is also easy with the design I advocate (duck-typing). Given that the other ticket author is not happy with it, there are options (in no particular order):

  1. I proceed with my design, and include a workaround that does the rewriting of the domain ("boxing" things up), before doing the orbit and action computation. (Which is not very efficient, as it essentially involves recreating group domain and its GAP counterparts)
  1. We convince the other author that I am right, and drop the workaround.
  1. Somebody (not me) designs a G-set type/class framework to handle the issue.
  1. You convince me that I am wrong, and it's perfectly kosher to have domains like plain tuples (1,2,{1,2},...), and creating intransitive actions is more work than just appending permutations.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 6 years ago by ncohen

  1. We convince the other author that I am right, and drop the workaround.

Come on Dima, you do understand that on some instances your patch could return bad answers, do you ?

What's the problem with gessing the "depth" of input/output according to the value of action ?

Nathann

comment:31 in reply to: ↑ 30 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen:

  1. We convince the other author that I am right, and drop the workaround.

Come on Dima, you do understand that on some instances your patch could return bad answers, do you ?

On wrong instances, yes. On instances like (1,2,{1,2},...), which make no sense, because they lead to horrible bugs. Here is how.

Take, say, directed 3-cycle, and label its vertices, in the cyclic order, 1, 2, (1,2). So you get G=Z_3, the automorphism group of this digraph, acting on the domain V=(1,2,(1,2)). Next, ask for the orbit of the arc (1,2) of the digraph under G. OK, fine, it is A=((1,2),(2,(1,2)),((1,2),1). Now, note that the intersection of V and A equals {(1,2)}. The intersection of two distinct orbits of a group is not empty...

Would Évariste Galois raise from his grave and chase the designer of this? :–)

Edit. Or implement the action on tuples of tuples, and ask for the orbit of ((1,2),(1,2)).

What's the problem with gessing the "depth" of input/output according to the value of action ?

this won't fix the bug above.

Dima

Nathann

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

comment:32 in reply to: ↑ 31 ; follow-up: Changed 6 years ago by ncohen

Take, say, directed 3-cycle, and label its vertices, in the cyclic order, 1, 2, (1,2). So you get G=Z_3, the automorphism group of this digraph, acting on the domain V=(1,2,(1,2)). Next, ask for the orbit of the arc (1,2) of the digraph under G. OK, fine, it is A=((1,2),(2,(1,2)),((1,2),1). Now, note that the intersection of V and A equals {(1,2)}. The intersection of two distinct orbits of a group is not empty...

Ahahaahah. Yeah, it looks like you need to guess somewhere that the element (1,2) is not equal to the set containing the two elements 1 and 2 :-)

Would Évariste Galois raise from his grave and chase the designer of this? :–)

If he does I will help !

What's the problem with gessing the "depth" of input/output according to the value of action ?

this won't fix the bug above.

That's true. But the two problems are totally unrelated, though ! In the current state of things, guessing the value of action as Volker said and translating input/output according to what is what we can do best. Then you will have to hand your beloved groups over to the combinat guys who will make a hell out of it, and I hope that everything I need from groups will be written before they make this impossible to use with their parent/element framework.

Nathann

comment:33 in reply to: ↑ 32 ; follow-up: Changed 6 years ago by dimpase

Replying to ncohen: [...]

Ahahaahah. Yeah, it looks like you need to guess somewhere that the element (1,2) is not equal to the set containing the two elements 1 and 2 :-)

easy, as I said: just replace domain elements 1 and 2 by (1) and (2), and everything all of a sudden works. The alternative is to give yourself over to the category theory and practice...

Would Évariste Galois raise from his grave and chase the designer of this? :–)

If he does I will help !

I won't bet on this, for he might get hold of more powerful weapon than he used during that unfortunate duel, you know :–)

What's the problem with gessing the "depth" of input/output according to the value of action ?

this won't fix the bug above.

That's true. But the two problems are totally unrelated, though !

of course they are related, as the code in question works fine on the sane inputs, and there is nothing to fix then.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 6 years ago by ncohen

easy, as I said: just replace domain elements 1 and 2 by (1) and (2), and everything all of a sudden works. The alternative is to give yourself over to the category theory and practice...

Yeah but it's the same problem then. My vertices are not the elements of the automorphism group. That's a problem.

Nathann

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

Replying to ncohen:

easy, as I said: just replace domain elements 1 and 2 by (1) and (2), and everything all of a sudden works. The alternative is to give yourself over to the category theory and practice...

Yeah but it's the same problem then. My vertices are not the elements of the automorphism group. That's a problem.

sorry, I don't understand. I never suggested labeling anything by group elements.

Nathann

comment:36 Changed 6 years ago by ncohen

You can also solve your problem above by saying that the intersection of an orbit of a vertex and the orbit of an edge is empty. And you can guess that because you computed the first with action="OnPoints" and the second with action="OnSets".

Nathann

comment:37 in reply to: ↑ 35 Changed 6 years ago by ncohen

sorry, I don't understand. I never suggested labeling anything by group elements.

Then how do you apply your problem to this patch ? How do we solve this ? I claim that there is nothing wrong going on for as long as there is an "action" argument somewhere which defines how output and input are to be interpreted.

Nathann

Last edited 6 years ago by ncohen (previous) (diff)

comment:38 Changed 6 years ago by ncohen

  • Description modified (diff)

Here's a new patch for what this ticket initially addressed, after the looooooong discussion on sage-devel [1]. #14283 and (more importantly) #14319 depend on this ticket !

Nathann

[1] https://groups.google.com/d/topic/sage-devel/LRZULwy5KYI/discussion

Last edited 6 years ago by ncohen (previous) (diff)

comment:39 Changed 6 years ago by ncohen

Apply trac_14291-v2.patch

Changed 6 years ago by ncohen

comment:40 in reply to: ↑ 29 Changed 6 years ago by vbraun

Can we make orbit return a tuple intstead of a list? As I mentioned before, @cached_methods must not return mutable containers.

comment:41 Changed 6 years ago by vbraun

  • Authors changed from Nathann Cohen to Nathann Cohen, Volker Braun
  • Description modified (diff)
  • Reviewers set to Volker Braun

comment:42 Changed 6 years ago by vbraun

I've cleaned up some stuff and beautified the output containers (as we talked about before):

sage: S3 = groups.permutation.Symmetric(3) 
sage: S3.orbit((1,2), action = "OnSets") 
({1, 2}, {2, 3}, {1, 3}) 
sage: S3.orbit((1,2), action = "OnTuples") 
((1, 2), (2, 3), (2, 1), (3, 1), (1, 3), (3, 2)) 

I'm fine with everything else, so if you agree with the reviewer patch feel free to set it to positive review.

For the patchbot: Apply trac_14291-v2.patch, trac_14291_reviewer.patch

comment:43 Changed 6 years ago by ncohen

Helloooooooooooooooo !!!

There is a "permutatino" somewhere. Could you also replace x = sorted(x) with x.sort()` ?

This being said, your patch does not apply on my version of Sage... But I still use beta1, perhaps that's the reason why.

Nathann

Changed 6 years ago by vbraun

Updated patch

comment:44 Changed 6 years ago by vbraun

Ok, fixed. Applies cleanly on sage-5.9.beta5.

comment:45 Changed 6 years ago by vbraun

Last edited 6 years ago by vbraun (previous) (diff)

comment:46 follow-up: Changed 6 years ago by vbraun

Ok all doctests pass on sage-5.9.beta5.

Patchbot: Apply trac_14291-v2.patch, trac_14291_reviewer.patch

comment:47 in reply to: ↑ 46 Changed 6 years ago by ncohen

Ok all doctests pass on sage-5.9.beta5.

Doesn't apply on my beta5 :-/

Do you have anything else applied ?

Nathann

comment:48 Changed 6 years ago by vbraun

Ok was based on an older version of your patch (we can't switch to git fast enough ;-)

Works now.

comment:49 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

Well, then... :-)

Nathann

comment:50 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-5.10

comment:51 Changed 6 years ago by dimpase

I'm glad to hear that it's done, and sorry for dropping silent on this - too many deadlines lately.

Dima

comment:52 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The documentation doesn't build properly:

dochtml.log:[groups   ] /mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/groups/perm_gps/permgroup.py:docstring of sage.groups.perm_gps.permgroup.PermutationGroup_generic.orbit:12: ERROR: Unknown target name: "http://www.gap-system.org/manuals/doc/ref/chap41.html".

comment:53 Changed 6 years ago by vbraun

  • Status changed from needs_work to positive_review

I've replaced the link (which is not stable as the GAP chapter numbers can change) with a reference to our gap.help('Group Actions') command.

Changed 6 years ago by vbraun

Updated patch

comment:54 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:55 Changed 6 years ago by ncohen

Cooooooooool :-D

Nathann

comment:56 Changed 6 years ago by dimpase

Shouldn't "OnSets" return sorted things? See e.g. the related issue on #14283.

Note: See TracTickets for help on using tickets.