Tableau_class.__div__, Partition_class.__div__ are checking domination when they should check inclusion

[hughrthomas]

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

Depends on #9265

Component: combinatorics

Keywords: tableau

Author: Hugh Thomas

Reviewer: Mike Hansen, Andrew Mathas

Merged: sage-5.5.beta1

Issue created by migration from https://trac.sagemath.org/ticket/12943

[mwhansen]

comment:3

Looks good to me.

[mwhansen]

Reviewer: Mike Hansen

[hughrthomas]

comment:4

Thanks, Mike!

[jdemeyer]

comment:5

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

[nthiery]

comment:6

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.

[AndrewMathas]

comment:8

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

[hughrthomas]

Attachment: trac_12943-skew-partition-construction-bug-ht.patch.gz

[hughrthomas]

comment:9

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

[nthiery]

comment:10

Replying to @hughrthomas:

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

[jdemeyer]

comment:11

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

[AndrewMathas]

comment:12

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

[hughrthomas]

comment:13

@sheerluck: 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.

[AndrewMathas]

comment:14

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.

[hughrthomas]

comment:16

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.

[jdemeyer]

Dependencies: #9265

[AndrewMathas]

Changed reviewer from Mike Hansen to Mike Hansen, Andrew Mathas

[jdemeyer]

Merged: sage-5.5.beta1