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: |
Description (last modified by )
Implements the following methods for partitions:
- A method which returns the list of cells in the hook of (i,j).
- A method which returns the list of cells in the rim hook of (i,j).
- A method which returns the partition removing the rim hook of (i,j).
- A method which returns the list of cells with content k.
Attachments (2)
Change History (12)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Description modified (diff)
- Keywords sd40 added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- 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
- Reviewers set to Andrew Mathas
- Status changed from needs_review to needs_work
comment:4 Changed 10 years ago by
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.
comment:5 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:6 follow-up: ↓ 8 Changed 8 years ago by
- 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
- Milestone changed from sage-6.1 to sage-6.2
comment:8 in reply to: ↑ 6 Changed 8 years ago by
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
comment:9 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
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
andrim_hook_cells
to return the empty list andremove_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.
0<=k+c and k+c<=p[k]
incells_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.remove_rim_hook()
function with the list compression:This turns out to be significantly faster:
Here, the
remove_rim_hook
function is as originally defined in the patch andf
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 assumingmake ptestlong
passes, I can change this to a positive review.