Opened 11 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: |
Description (last modified by )
In permgroup_named.py, add a method major_index for the SymmetricGroup?(n)
Attachments (4)
Change History (29)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
Changed 11 years ago by
Changed 11 years ago by
comment:2 Changed 11 years ago by
- Reviewers set to Mike Hansen
comment:3 Changed 11 years ago by
- 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
- 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
- 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
- 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
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
comment:8 Changed 10 years ago by
- 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
- 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: ↓ 12 Changed 10 years ago by
- 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
- Description modified (diff)
- Status changed from needs_work to positive_review
comment:12 in reply to: ↑ 10 Changed 10 years ago by
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
- Description modified (diff)
comment:14 follow-up: ↓ 16 Changed 10 years ago by
- Status changed from positive_review to needs_info
May I assume that the description is wrong and that three patches need to be applied?
comment:15 Changed 10 years ago by
- Description modified (diff)
comment:16 in reply to: ↑ 14 ; follow-up: ↓ 18 Changed 10 years ago by
- 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
- Status changed from needs_review to positive_review
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 20 Changed 10 years ago by
- 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
- Status changed from needs_work to needs_review
comment:20 in reply to: ↑ 18 Changed 10 years ago by
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
- 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
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
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
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
- Merged in set to sage-4.7.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
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?