Opened 10 years ago

Closed 10 years ago

#9949 closed enhancement (fixed)

Add major index (polynomial) for the symmetric group

Reported by: nborie Owned by: nborie
Priority: major Milestone: sage-4.7
Component: combinatorics Keywords: major, index, generating, polynomial, permutation
Cc: sage-combinat, nthiery Merged in: sage-4.7.alpha4
Authors: Nicolas Borie Reviewers: Mike Hansen, Jason Bandlow
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by nthiery)

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

Apply: trac_9949_major_index_really_final-nb.patch

Attachments (4)

trac_9949_major_index_finite_permutation_group-nb.patch (5.1 KB) - added by nborie 10 years ago.
trac_9949_major_index_finite_permutation_group-review-mh.patch (3.2 KB) - added by mhansen 10 years ago.
trac_9949_major_index_final-nb.patch (1.6 KB) - added by nborie 10 years ago.
trac_9949_major_index_really_final-nb.patch (1.6 KB) - added by nthiery 10 years ago.
Really final version, with ticket number

Download all attachments as: .zip

Change History (29)

comment:1 Changed 10 years ago by nborie

  • Authors set to nborie
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by mhansen

  • Authors changed from nborie to Nicolas Borie
  • Reviewers set to Mike Hansen

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?

comment:3 Changed 10 years ago by nborie

  • Status changed from needs_review to needs_info

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

comment:4 Changed 10 years ago by nborie

  • Description modified (diff)
  • Summary changed from Add major index (generating polynomial) for the category of finite permutation groups to Add major index (polynomial) for the symmetric group

comment:5 Changed 10 years ago by nborie

  • Status changed from needs_info to needs_review

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.

comment:6 Changed 10 years ago by jbandlow

  • Status changed from needs_review to needs_info

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.

comment:7 Changed 10 years ago by nborie

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!

Changed 10 years ago by nborie

comment:8 Changed 10 years ago by nborie

  • Status changed from needs_info to needs_review

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.

comment:9 Changed 10 years ago by jbandlow

  • Milestone set to sage-4.7
  • Reviewers changed from Mike Hansen to Mike Hansen, Jason Bandlow
  • Status changed from needs_review to positive_review

This looks good. Thanks, Nicolas.

comment:10 follow-up: Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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.

comment:11 Changed 10 years ago by nthiery

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

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

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!

comment:13 Changed 10 years ago by nthiery

  • Description modified (diff)

comment:14 follow-up: Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_info

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

Changed 10 years ago by nthiery

Really final version, with ticket number

comment:15 Changed 10 years ago by nthiery

  • Description modified (diff)

comment:16 in reply to: ↑ 14 ; follow-up: Changed 10 years ago by nthiery

  • Status changed from needs_info to needs_review

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!

comment:17 Changed 10 years ago by nthiery

  • Status changed from needs_review to positive_review

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

  • Status changed from positive_review to needs_work

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

comment:19 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:20 in reply to: ↑ 18 Changed 10 years ago by nthiery

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?

comment:21 Changed 10 years ago by jbandlow

  • Status changed from needs_review to positive_review

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.

comment:22 Changed 10 years ago by jbandlow

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

Apply only trac_9949_major_index_really_final-nb.patch

comment:23 Changed 10 years ago by jdemeyer

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

comment:24 Changed 10 years ago by nborie

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.

comment:25 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.