Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

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

trac_12943-skew-partition-construction-bug-ht.patch (2.0 KB) - added by hthomas 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by hthomas

  • Priority changed from major to minor

comment:2 Changed 7 years ago by hthomas

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by mhansen

  • Reviewers set to Mike Hansen
  • Status changed from needs_review to positive_review

Looks good to me.

comment:4 Changed 7 years ago by hthomas

Thanks, Mike!

comment:5 Changed 7 years ago by jdemeyer

  • 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 nthiery

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 hthomas

  • Status changed from needs_work to needs_review

comment:8 Changed 7 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 7 years ago by andrew.mathas (previous) (diff)

comment:9 follow-up: Changed 7 years ago by hthomas

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: Changed 7 years ago by nthiery

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 jdemeyer

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 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 7 years ago by andrew.mathas (previous) (diff)

comment:13 Changed 7 years ago by hthomas

@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 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 7 years ago by andrew.mathas (previous) (diff)

comment:15 Changed 7 years ago by andrew.mathas

  • Status changed from needs_review to positive_review

comment:16 Changed 7 years ago by hthomas

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 jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:18 Changed 7 years ago by jdemeyer

  • Dependencies set to #9265
  • Milestone changed from sage-5.5 to sage-pending

comment:19 Changed 7 years ago by andrew.mathas

  • 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 jdemeyer

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 7 years ago by jdemeyer

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