#12943 closed defect (fixed)
Tableau_class.__div__, Partition_class.__div__ are checking domination when they should check inclusion
Reported by: | Hugh Thomas | Owned by: | Sage Combinat CC user |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.5 |
Component: | combinatorics | Keywords: | tableau |
Cc: | Merged in: | sage-5.5.beta1 | |
Authors: | Hugh Thomas | Reviewers: | Mike Hansen, Andrew Mathas |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #9265 | Stopgaps: |
Description
If rho and mu are partitions, rho/mu is supposed to return the skew partition rho/mu. It tries to check that mu is of the right shape for it to be able to do this, but it does this check using the dominates() method, which tests dominance order not inclusion order. This is not so serious, in that if rho contains mu, then it does also dominate mu, so it will always let correct calculations go through. The error is then caught by SkewPartition?, which realizes it's been handed invalid data. So this is not actually so bad.
However, the same thing happens when T is a tableau and you calculate T/mu, and there, it causes a "list index out of range".
sage: rho = Partition((3,2,1)) sage: mu = Partition((2,1,1,1)) sage: rho/mu ValueError: invalid skew partition: [[3, 2, 1], [2, 1, 1, 1]] sage: t=Tableau([[1,2,3],[4,5],[6]]) sage: t/mu IndexError: list index out of range
Attachments (1)
Change History (22)
comment:1 Changed 10 years ago by
Priority: | major → minor |
---|
comment:2 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 10 years ago by
Reviewers: | → Mike Hansen |
---|---|
Status: | needs_review → positive_review |
comment:5 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
You need to add a proper commit message to the patch, use hg qrefresh -e
for that. (Note: multiple lines are allowed, but make sure the first line makes sense by itself).
comment:6 Changed 10 years ago by
You might want to add a doctest to tableau for the value error by the way. Note: I just moved your patch up the Sage-Combinat queue.
comment:7 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
comment:8 Changed 10 years ago by
This looks good. The only change that I woiuld recommend is improving the grammar. Rather than
To form a skew partition p/q, it must be that q is contained in p
I suggest
To form the skew partition p/q, q must be contained in p
or even more simply:
the partition q must be contained in p
Once you have decided what's best then, following Mike's recommendation, please set this to a positive review.
Andrew
Changed 10 years ago by
Attachment: | trac_12943-skew-partition-construction-bug-ht.patch added |
---|
comment:9 follow-up: 10 Changed 10 years ago by
Hi Andrew--
I improved the grammar as you requested. I also realized that I had somehow gotten things out of order in the tableau patch -- it was checking that the inputs were sensible after it had started processing them, rather than before. I fixed this too. I would therefore prefer that you set the positive review yourself, if you don't mind.
Thanks for reviewing this. I apologize for wasting everyone's time with this ticket, which is a rather minuscule improvement. I will try to restrain myself from indulging in similar tidying impulses in the future.
cheers,
Hugh
comment:10 follow-up: 11 Changed 10 years ago by
Replying to hthomas:
Thanks for reviewing this. I apologize for wasting everyone's time with this ticket, which is a rather minuscule improvement. I will try to restrain myself from indulging in similar tidying impulses in the future.
Don't! Sage also needs those small improvements :-) On the other hand, given that there was some refactoring going on with tableaux, an alternative would have been to propose a little patch to be folded into Andrew's ticket rather than creating a new ticket. But we don't want too big tickets either, so it's just a question of balance.
Thanks!
Nicolas
comment:11 Changed 10 years ago by
Replying to nthiery:
But we don't want too big tickets either
+1
I usually prefer separate tickets for separate issues, which is much easier to review (but needs slightly more effort for the author).
comment:12 Changed 10 years ago by
Hi Hugh.
The patch looks good. I ran tests with the initial patch -- they passed -- but haven't done retested the patch. The patchbot doesn't seem to want to run the tests for me either and I am not sure how to make it do so.
Assuming that the tests all pass, I give it a positive review.
Also, I agree with Nicolas and Jeroen that large patches should be avoided (even though I seem to have mainly large patches in the queue...)
Andrew
--
For the patchbot:
Apply trac_12943-skew-partition-construction-bug-ht.patch
comment:13 Changed 10 years ago by
@Andrew: I have kicked the patchbot, so hopefully it will now test the patch. When (/if) it's green, I will set a positive review on your behalf. Thanks!
@Nicolas: Thanks for the encouragement.
comment:14 Changed 10 years ago by
The patchbot didn't seem interested in checking so I have just ran the tests "by hand". Everything passes so I'll given this a positive review.
comment:15 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
comment:16 Changed 10 years ago by
Thanks, Andrew!
I tried to run the patchbot on my machine, but didn't do it right, which is the explanation for the red blob that has appeared. I will try to do it again, right, and make the red blob go away.
comment:17 Changed 10 years ago by
Milestone: | sage-5.4 → sage-5.5 |
---|
comment:18 Changed 10 years ago by
Dependencies: | → #9265 |
---|---|
Milestone: | sage-5.5 → sage-pending |
comment:19 Changed 10 years ago by
Milestone: | sage-pending → sage-5.5 |
---|---|
Reviewers: | Mike Hansen → Mike Hansen, Andrew Mathas |
comment:20 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
comment:21 Changed 10 years ago by
Merged in: | → sage-5.5.beta1 |
---|
Looks good to me.