Ticket #5790 (closed enhancement: fixed)

Opened 4 years ago

Last modified 4 years ago

[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, jbandlow, saliola Work issues:
Report Upstream: Reviewers: Jason Bandlow, Franco Saliola, Nicolas M. Thiéry
Authors: Andrew Mathas Merged in: sage-4.1.1.alpha0
Dependencies: Stopgaps:

Description (last modified by andrew.mathas) (diff)

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

partition-update-AM.patch Download (21.5 KB) - added by andrew.mathas 4 years ago.
partition-update-AM.2.2.patch Download (71 bytes) - added by andrew.mathas 4 years ago.
partition-update-AM.2.patch Download (71 bytes) - added by andrew.mathas 4 years ago.
5790.2.patch Download (23.0 KB) - added by jbandlow 4 years ago.
trac_5790-review.patch Download (4.7 KB) - added by saliola 4 years ago.
Apply only 5790.2.patch and this patch (in this order)
trac_5790-partition.patch Download (29.4 KB) - added by nthiery 4 years ago.
Apply only this one
trac_5790_review_nt.patch Download (15.7 KB) - added by nthiery 4 years ago.
For the record. Don't apply.

Change History

comment:1 Changed 4 years ago by andrew.mathas

  • Component changed from algebra to combinatorics
  • Description modified (diff)

comment:2 Changed 4 years ago by andrew.mathas

  • Type changed from defect to task

comment:3 Changed 4 years ago by saliola

  • Summary changed from Updating some quirks in partition.py to [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 4 years ago by andrew.mathas

  • Summary changed from [with patch; needs work] Updating some quirks in partition.py to [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 4 years ago by andrew.mathas

Changed 4 years ago by andrew.mathas

Changed 4 years ago by andrew.mathas

comment:5 Changed 4 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 4 years ago by jbandlow

comment:6 Changed 4 years ago by jbandlow

  • Cc jbandlow, saliola added
  • Reviewers set to Jason Bandlow, Andrew Saliola
  • Summary changed from [with patch; needs review II] Updating some quirks in partition.py to [with patch; needs review] Updating some quirks in partition.py
  • Authors set to Andrew Mathas

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 4 years ago by jbandlow

  • Reviewers changed from Jason Bandlow, Andrew Saliola to Jason Bandlow, Franco Saliola

comment:8 Changed 4 years ago by ddrake

  • Type changed from task to enhancement

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 4 years ago by saliola

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

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

I am looking at it. Should be done tonight.

comment:11 Changed 4 years ago by nthiery

  • Reviewers changed from Jason Bandlow, Franco Saliola to Jason Bandlow, Franco Saliola, Nicolas M. Thiéry
  • Summary changed from [with patch; needs review] Updating some quirks in partition.py to [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 4 years ago by nthiery

Apply only this one

Changed 4 years ago by nthiery

For the record. Don't apply.

comment:12 Changed 4 years ago by saliola

I give a positive review to Nicolas's changes.

comment:13 Changed 4 years ago by mvngu

  • Status changed from new to closed
  • Resolution set to fixed
  • Merged in set to sage-4.1.1.alpha0
  • Summary changed from [with patch; positive review] Updating some quirks in partition.py to [with patch, positive review] Updating some quirks in partition.py

Merged trac_5790-partition.patch.

comment:14 Changed 4 years ago by mvngu

  • Milestone changed from sage-combinat to sage-4.1.1
Note: See TracTickets for help on using tickets.