Opened 10 years ago

Last modified 8 years ago

#13235 needs_work enhancement

Implement some new features for partitions (hook_cells, etc.)

Reported by: numata Owned by: sage-combinat
Priority: major Milestone: sage-6.4
Component: combinatorics Keywords: partitions, combinatorics, sd40
Cc: sage-combinat, aschilling Merged in:
Authors: NUMATA, Yasuhide Reviewers: Andrew Mathas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by numata)

Implements the following methods for partitions:

  1. A method which returns the list of cells in the hook of (i,j).
  2. A method which returns the list of cells in the rim hook of (i,j).
  3. A method which returns the partition removing the rim hook of (i,j).
  4. A method which returns the list of cells with content k.

Attachments (2)

trac_13235-hook_cells_for_parition-nu.patch (3.8 KB) - added by numata 10 years ago.
trac_13235-review-patch-am.patch (7.1 KB) - added by andrew.mathas 10 years ago.
Reivew patch (forgot to save pervious version!)

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by numata

  • Description modified (diff)
  • Keywords sd40 added
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by kini

  • Summary changed from Implementing some new feature for partions (hook_cells..) to Implement some new features for partitions (hook_cells, etc.)

comment:3 Changed 10 years ago by andrew.mathas

  • Reviewers set to Andrew Mathas
  • Status changed from needs_review to needs_work

Everything looks good although I have one comment: if the cell (i,j) is NOT contained in the diagram of the partition then all of these functions currently return an error, however, another possibility would be for the functions hook_cells and rim_hook_cells to return the empty list and remove_rim_hook to return the original partition. This is certainly compatible with the definitions. Having said this, what you have done is probably the best choice as it is likely to catch unintentional errors of people using these functions.

The following review patch makes the following minor changes.

  1. Fixes the typos: "Rertuns" -> "Returns" and "The cells is not in the diagram" -> "The cell is not in the diagram"
  1. Expands some of the doc strings so that they are more informative.
  1. Combines the two tests 0<=k+c and k+c<=p[k] in cells_with_content() into a single statement: 0 <= k+c < p[k]. I also don't see the value in defining a new variable, p=self, so I have deleted this.
  1. It replaces the remove_rim_hook() function with the list compression:
    return Partition([p[r] if (r<i or p[r]<=j) else j if (r==len(p)-1 or p[r+1]<=j) else p[r+1]-1 for r in range(len(p))])
    

This turns out to be significantly faster:

sage: timeit( '[mu.remove_rim_hook(i,j) for mu in Partitions(10) for (i,j) in mu.cells()]' )
5 loops, best of 3: 48.4 ms per loop
sage: timeit( '[f(mu,i,j) for mu in Partitions(10) for (i,j) in mu.cells()]' )
25 loops, best of 3: 10.5 ms per loop

Here, the remove_rim_hook function is as originally defined in the patch and f is the list compression version.

A similar list compression could be used in the function rim_hook_cells() but I haven't done this.

Finally, there are some doc-test errors in partition.py but these are caused by the changes to depreciation in 5.2, so they are not caused by your patch and I have ignored them.

I have left the status as needs review as you should review my suggestions above. Once you have looked at these, and assuming make ptestlong passes, I can change this to a positive review.

Last edited 10 years ago by andrew.mathas (previous) (diff)

Changed 10 years ago by andrew.mathas

Reivew patch (forgot to save pervious version!)

comment:4 Changed 10 years ago by andrew.mathas

Another comment: it would be good to have an analogous add_rim_hook function which returns the partition obtained by adding a rim hook of a given length and with a specified head or foot node.

Last edited 10 years ago by andrew.mathas (previous) (diff)

comment:5 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 follow-up: Changed 8 years ago by darij

  • Cc aschilling added

What happened to this? Seems bit-rotten now. Should it be revived?

The methods seem useful.

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 in reply to: ↑ 6 Changed 8 years ago by andrew.mathas

Replying to darij:

What happened to this? Seems bit-rotten now. Should it be revived?

The methods seem useful.

Probably the original author lost interest. I think that methods for adding and removing rim hooks would be useful but I am not convinced about the need for the other methods. The problem is that partitions already have a huge number of methods and this makes it hard for the uninitiated to find the methods that they actually need. In addition to sage's global namespace, we probably also need to be careful that we do not over pollute the "method namespace" of the popular classes.

Of course, I have some private code that adds still more methods to the partition class and others might think that these methods are equally useless:) In this respect, it's unfortunate that python doesn't allow you to dynamically expand classes so that the user is only exposed to the "really useful" methods by default. (Actually, I have a decorator which makes this feasible, but python purists might argue against my using this in sage:)

If you want to revive this patch I'd be happy to review a working version.

Andrew

Last edited 8 years ago by andrew.mathas (previous) (diff)

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 8 years ago by vbraun_spam

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