Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#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:

Status badges

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)

trac_12943-skew-partition-construction-bug-ht.patch (2.0 KB) - added by Hugh Thomas 10 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Hugh Thomas

Priority: majorminor

comment:2 Changed 10 years ago by Hugh Thomas

Status: newneeds_review

comment:3 Changed 10 years ago by Mike Hansen

Reviewers: Mike Hansen
Status: needs_reviewpositive_review

Looks good to me.

comment:4 Changed 10 years ago by Hugh Thomas

Thanks, Mike!

comment:5 Changed 10 years ago by Jeroen Demeyer

Status: positive_reviewneeds_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 Nicolas M. Thiéry

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 Hugh Thomas

Status: needs_workneeds_review

comment:8 Changed 10 years ago by Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

Changed 10 years ago by Hugh Thomas

comment:9 Changed 10 years ago by Hugh Thomas

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 ; Changed 10 years ago by Nicolas M. Thiéry

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 10 years ago by Jeroen Demeyer

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 Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

comment:13 Changed 10 years ago by Hugh Thomas

@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 Andrew Mathas

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.

Last edited 10 years ago by Andrew Mathas (previous) (diff)

comment:15 Changed 10 years ago by Andrew Mathas

Status: needs_reviewpositive_review

comment:16 Changed 10 years ago by Hugh Thomas

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 Jeroen Demeyer

Milestone: sage-5.4sage-5.5

comment:18 Changed 10 years ago by Jeroen Demeyer

Dependencies: #9265
Milestone: sage-5.5sage-pending

comment:19 Changed 10 years ago by Andrew Mathas

Milestone: sage-pendingsage-5.5
Reviewers: Mike HansenMike Hansen, Andrew Mathas

comment:20 Changed 10 years ago by Jeroen Demeyer

Resolution: fixed
Status: positive_reviewclosed

comment:21 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.5.beta1
Note: See TracTickets for help on using tickets.