#12943 closed defect (fixed)
Tableau_class.__div__, Partition_class.__div__ are checking domination when they should check inclusion
Reported by: | hthomas | Owned by: | sage-combinat |
---|---|---|---|
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 7 years ago by
- Priority changed from major to minor
comment:2 Changed 7 years ago by
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Reviewers set to Mike Hansen
- Status changed from needs_review to positive_review
comment:4 Changed 7 years ago by
Thanks, Mike!
comment:5 Changed 7 years ago by
- Status changed from positive_review to 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 7 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 7 years ago by
- Status changed from needs_work to needs_review
comment:8 Changed 7 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 7 years ago by
comment:9 follow-up: ↓ 10 Changed 7 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 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 7 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 in reply to: ↑ 10 Changed 7 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 7 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 7 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 7 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 7 years ago by
- Status changed from needs_review to positive_review
comment:16 Changed 7 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 7 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:18 Changed 7 years ago by
- Dependencies set to #9265
- Milestone changed from sage-5.5 to sage-pending
comment:19 Changed 7 years ago by
- Milestone changed from sage-pending to sage-5.5
- Reviewers changed from Mike Hansen to Mike Hansen, Andrew Mathas
comment:20 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
comment:21 Changed 7 years ago by
- Merged in set to sage-5.5.beta1
Looks good to me.