Add major index (polynomial) for the symmetric group

[sagetrac-nborie]

In permgroup_named.py, add a method major_index for the SymmetricGroup(n)

Apply: attachment: trac_9949_major_index_really_final-nb.patch

CC: @sagetrac-sage-combinat @nthiery

Component: combinatorics

Keywords: major, index, generating, polynomial, permutation

Author: Nicolas Borie

Reviewer: Mike Hansen, Jason Bandlow

Merged: sage-4.7.alpha4

Issue created by migration from https://trac.sagemath.org/ticket/9949

[sagetrac-nborie]

Author: nborie

[sagetrac-nborie]

Attachment: trac_9949_major_index_finite_permutation_group-nb.patch.gz

[mwhansen]

Changed author from nborie to Nicolas Borie

[mwhansen]

Reviewer: Mike Hansen

[mwhansen]

comment:2

Attachment: trac_9949_major_index_finite_permutation_group-review-mh.patch.gz

I've added a review patch which fixes a few minor things. Other than that, it looks good to me. Do you want to fold the patches together, put the new one up, and I can give it positive review?

[sagetrac-nborie]

comment:3

Yes, I definitely agree with yours corrections. But before finalizing this ticket, we need some informations. Nicolas told me that it is not really reasonable to implement this feature in this category. We don't really know if major index is defined for any Finite Permutation Group. Let's discuss this on sage-combinat-devel.

http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/38a0e71e61ca6231

Thank you very much Mike for your patch, I also should have open this discussion earlier. Sorry for that...

[sagetrac-nborie]

comment:5

After discussions, I realized that it is reasonable to define major_index only for the symmetric group. So I moved the method in the proper place. I also integrated all remarks and code corrections from the patch of Mike.

For Buildbot / reviewer / ... :

apply trac_9949_major_index_final-nb.patch

It is now ready for review.

[jbandlow]

comment:6

Hi Nicolas,

If this only applies to symmetric groups, shouldn't it just return

sage.combinat.q_analogues.q_factorial(n)

?

This would be much more efficient than enumerating over the group.

[sagetrac-nborie]

comment:7

Hy Jason

You are definitely right! I didn't know this module about q_analogues. I am going to change it and just make major_cycle point to the right q_analogue. As q_analogues is not imported by default, this ticket will just consist in building a shortcut...

Thanks for having regarded this!

[sagetrac-nborie]

Attachment: trac_9949_major_index_final-nb.patch.gz

[sagetrac-nborie]

comment:8

I update the patch after your last comment Jason. At the end, this method is just a shortcut pointing to the q-analogue of factorial n. As q_analogues are not imported by default and calling SymmetricGroup(n).major_index() seems natural, I think it is good like this.

[jbandlow]

Changed reviewer from Mike Hansen to Mike Hansen, Jason Bandlow

[jbandlow]

comment:9

This looks good. Thanks, Nicolas.

[nthiery]

comment:12

Replying to @jdemeyer:

Please change the commit message of the patches to something meaningful. Make sure the ticket number appears on the first line of the commit message.

Oops, I should have caught this. Fixed!

[jdemeyer]

comment:14

May I assume that the description is wrong and that three patches need to be applied?

[nthiery]

Really final version, with ticket number

[nthiery]

comment:15

Attachment: trac_9949_major_index_really_final-nb.patch.gz

[nthiery]

comment:16

Replying to @jdemeyer:

May I assume that the description is wrong and that three patches need to be applied?

Sorry, I uploaded the wrong file from the sage-combinat queue, which probably caused the confusion. I confirm that only the advertised patch shall be applied.

Thanks!

[jdemeyer]

comment:18

Replying to @nthiery:

I confirm that only the advertised patch shall be applied.

This statement is a non-trivial change to the ticket and needs to be reviewed (since your patch is only a subset of the previous patches).

[nthiery]

comment:20

Replying to @jdemeyer:

Replying to @nthiery:

I confirm that only the advertised patch shall be applied.

This statement is a non-trivial change to the ticket and needs to be reviewed (since your patch is only a subset of the previous patches).

Sorry if there is any confusion, but the reduction to a subset dates back from 7 weeks ago, and was already given a positive review by Jason Bandlow. I only changed the patch header from trac_9949_major_index_final-nb.patch. So I think it should be positive review.

Do you mind setting it back if we are now on the same line?

[jbandlow]

comment:21

I confirm that Nicolas Thiery's changes were only to the header of the patch previously given a positive review by me. I am resetting the status to positive review. My apologies for missing the incomplete commit message in my first review.

[jbandlow]

comment:22

To be sure I am clear, the ticket description is correct:

Apply only trac_9949_major_index_really_final-nb.patch

[jdemeyer]

comment:23

I understand everything now, but bear in mind that it is very important to write in the ticket description which patches have to be applied if it's not obvious. If it weren't for the missing commit message, I would have merged all three patches instead of only the last one (and we would never have known that we did something wrong).

[sagetrac-nborie]

comment:24

Sorry for all of that,

It is a 7 weeks old patch and despite I read sage-devel (and advises in sage-devel like the use of hg qrefresh -e and other patch submitting procedures), I didn't have the reflex of checking all my submitted patch to verify they are conforms. It is not the first time I am making this mistake. Sorry, I will try to be very very conscientious the next time.

And I am on the way checking all I already put in trac the last months...

Thanks for your help to all of you.

[jdemeyer]

Merged: sage-4.7.alpha4