#13235: Implement some new features for partitions (hook_cells, etc.)
---------------------------------------------------+------------------------
Reporter: numata | Owner:
sage-combinat
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-5.3
Component: combinatorics | Resolution:
Keywords: partitions, combinatorics, sd40 | Work issues:
Report Upstream: N/A | Reviewers: Andrew
Mathas
Authors: NUMATA, Yasuhide | Merged in:
Dependencies: | Stopgaps:
---------------------------------------------------+------------------------
Changes (by andrew.mathas):
* status: needs_review => needs_work
* reviewer: => Andrew Mathas
Comment:
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. What you have done is probably the best
choice, however, 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"
2. Expands some of the doc strings so that they are more informative.
3. 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.
4. 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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13235#comment:3>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
