Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#5790 closed enhancement (fixed)

[with patch, positive review] Updating some quirks in partition.py

Reported by: Andrew Mathas Owned by: Andrew Mathas
Priority: minor Milestone: sage-4.1.1
Component: combinatorics Keywords: partitions, cores, quotients
Cc: Sage Combinat CC user, Jason Bandlow, Franco Saliola Merged in: sage-4.1.1.alpha0
Authors: Andrew Mathas Reviewers: Jason Bandlow, Franco Saliola, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Andrew Mathas)

Dear All,

I have just pushed a patch to the combinat server which:

  • deprecates r_core, r_quotient (and k_core) in favour of core and quotient, respectively. I also made core return a partition rather than a list.
  • rewrites the Partition() calling function to use keywords rather than named arguments. In the process I deprecated the core_and_quotient argument replacing it with Partition(core=?,quotient=?).
  • deprecated partition_sign in favour of sign and replaced the previous call to gap with plus or minus one as required.

Almost all of the changes are to partition.py, however, the patch affects the following four files as they all called r_core or r_quotient:

  • sage/combinat/ktableau.py
  • sage/combinat/partition.py
  • sage/combinat/ribbon_tableau.py
  • sage/combinat/skew_partition.py

Not all of the doc tests pass, however, the problems seem to be caused elsewhere -- please let me know if I am wrong!

Andrew

Attachments (7)

partition-update-AM.patch (21.5 KB) - added by Andrew Mathas 14 years ago.
partition-update-AM.2.2.patch (71 bytes) - added by Andrew Mathas 14 years ago.
partition-update-AM.2.patch (71 bytes) - added by Andrew Mathas 14 years ago.
5790.2.patch (23.0 KB) - added by Jason Bandlow 13 years ago.
trac_5790-review.patch (4.7 KB) - added by Franco Saliola 13 years ago.
Apply only 5790.2.patch and this patch (in this order)
trac_5790-partition.patch (29.4 KB) - added by Nicolas M. Thiéry 13 years ago.
Apply only this one
trac_5790_review_nt.patch (15.7 KB) - added by Nicolas M. Thiéry 13 years ago.
For the record. Don't apply.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 14 years ago by Andrew Mathas

Component: algebracombinatorics
Description: modified (diff)

comment:2 Changed 14 years ago by Andrew Mathas

Type: defecttask

comment:3 Changed 14 years ago by Franco Saliola

Summary: Updating some quirks in partition.py[with patch; needs work] Updating some quirks in partition.py

The patch looks good. A couple of small comments.

  • The documentation for core still uses the terminology "r-core". Perhaps you can replace this with 'core' and add a note along the lines: 'sometimes called r-core and k-core in the literature'.
  • Same comment applies to the documentation for "r-quotient".

comment:4 Changed 14 years ago by Andrew Mathas

Summary: [with patch; needs work] Updating some quirks in partition.py[with patch; needs review II] Updating some quirks in partition.py

I've fixed the documentation problems...and also deprecated partition_associated, from_core_and_quotient and from_exp moving them all into Partition_class. All doc tests now pass.

Changed 14 years ago by Andrew Mathas

Attachment: partition-update-AM.patch added

Changed 14 years ago by Andrew Mathas

Changed 14 years ago by Andrew Mathas

Attachment: partition-update-AM.2.patch added

comment:5 Changed 14 years ago by Andrew Mathas

I noticed that from_core_and_quotient returned a list rather than a partition, so changed this as well...

Changed 13 years ago by Jason Bandlow

Attachment: 5790.2.patch added

comment:6 Changed 13 years ago by Jason Bandlow

Authors: Andrew Mathas
Cc: Jason Bandlow Franco Saliola added
Reviewers: Jason Bandlow, Andrew Saliola
Summary: [with patch; needs review II] Updating some quirks in partition.py[with patch; needs review] Updating some quirks in partition.py

I've rebased this to apply to 4.0.1; all doctests pass. I give a positive review to all of Andrew's changes, but I've left the status as 'needs review' so someone has a chance approve my changes. Let's get this in quickly before we need to rebase again! :)

comment:7 Changed 13 years ago by Jason Bandlow

Reviewers: Jason Bandlow, Andrew SaliolaJason Bandlow, Franco Saliola

comment:8 Changed 13 years ago by Dan Drake

Type: taskenhancement

I'm changing this from "task" to "enhancement" to make sure it shows up in the right queries and searches. Tasks somehow seem to get lost too easily.

Changed 13 years ago by Franco Saliola

Attachment: trac_5790-review.patch added

Apply only 5790.2.patch and this patch (in this order)

comment:9 Changed 13 years ago by Franco Saliola

Andrew's changes made from_exp and from_core_and_quotient methods of a partition, but they shouldn't be since they don't depend on the partition at all. It's strange, to me, to have the constructor work this way:

sage: p = Partition([])
sage: p.from_exp([3,2,1])
[3, 2, 2, 1, 1, 1]

sage: q = Partition([3,2,2,1,1,1])
sage: q.from_exp([3,2,1])
[3, 2, 2, 1, 1, 1]

I've reverted those changes in my review patch, otherwise it looks good.

To be clear: I do not object to gathering all the code for constructing partitions into one class. Perhaps a new PartitionConstructor class would be better? Either way, this is not the point of this ticket.

Someone needs to look over my changes, of course.

Apply only 5790.2.patch and trac_5790-review.patch.

comment:10 Changed 13 years ago by Nicolas M. Thiéry

I am looking at it. Should be done tonight.

comment:11 Changed 13 years ago by Nicolas M. Thiéry

Reviewers: Jason Bandlow, Franco SaliolaJason Bandlow, Franco Saliola, Nicolas M. Thiéry
Summary: [with patch; needs review] Updating some quirks in partition.py[with patch; positive review] Updating some quirks in partition.py

I rechecked the patches, merged them all together, did a couple minor doc fixes, renamed key_word into keyowrd, and merged in my patch on sage-combinat adding a doctest to the deprecation things for r_core and r_quotient (and fixed them, since they actually were broken) + updating of the llt code. The patch applies cleanly on 4.1. All tests pass. I consider it as good to go.

For the record. I am also about to attach a diff of my changes, should someone want to double checks.

Changed 13 years ago by Nicolas M. Thiéry

Attachment: trac_5790-partition.patch added

Apply only this one

Changed 13 years ago by Nicolas M. Thiéry

Attachment: trac_5790_review_nt.patch added

For the record. Don't apply.

comment:12 Changed 13 years ago by Franco Saliola

I give a positive review to Nicolas's changes.

comment:13 Changed 13 years ago by Minh Van Nguyen

Merged in: sage-4.1.1.alpha0
Resolution: fixed
Status: newclosed
Summary: [with patch; positive review] Updating some quirks in partition.py[with patch, positive review] Updating some quirks in partition.py

Merged trac_5790-partition.patch.

comment:14 Changed 13 years ago by Minh Van Nguyen

Milestone: sage-combinatsage-4.1.1
Note: See TracTickets for help on using tickets.