Sage: Ticket #13072: Implementation of PartitionTuple + some minor fixes to partition.py
https://trac.sagemath.org/ticket/13072
<p>
This patch implements the following classes:
</p>
<ul><li>PartitionTuple - returns a tuple of partitions
</li><li>PartitionTuples - factory class for all tuples of partitions
</li><li>!PartitionTuples_level - class of all tuples of partition of a fixed level
</li><li>!PartitionTuples_size - class of all tuples of partition of a fixed size
</li><li>!PartitionTuples_level_size - class of all tuples of partition of a fixed level and size. The first three of these are infinite enumerated classes whereas the last is finite. They all have iterators.
</li></ul><p>
The idea is to implement a fully function class for PartitionTuples as I currently need this together with a class for tuples of (standard) tableaux (coming soon).
</p>
<p>
PartitionTuples of level 1 are in natural bijection with Partitions so when given a 1-tuple of partitions, or a partition, PartitionTuples() returns the corresponding Partition. This works almost seamlessly, making it possible to almost ignore the distinction between Partitions() and PartitionTuples(). One exception is that the expected behaviour of
</p>
<pre class="wiki"> for component in mu:
do X
</pre><p>
is different for partitions and partition tuples (in the first case, you expect to loop over the parts of the partition and in the second over the components of the tuple). To get around this both classes now have a components() method so that you can uniformly write
</p>
<pre class="wiki">for nu in mu.components():
do X
</pre><p>
Improvements welcome!
</p>
<p>
In terms of implementation, for my use of these objects the <code>level</code> is more intrinsic than the size so I have set the syntax for the <a class="missing wiki">PartitionTuples?</a> classes as
</p>
<pre class="wiki">PartitionTuples(level=l, n=n)
</pre><p>
where <code>level</code> and <code>n</code> are both optional named arguments BUT level is specified first. Previously, <code>n</code> was given first and <code>level</code> second. I think that it makes much more sense this way around, but if people feel really strongly about this I will change it back. Previously, <code>level</code> was just called <code>k</code>, which is a fairly random variable whereas <code>level</code> makes sense in terms of categorification and higher level Fock spaces. (Replacing <code>n</code> with <code>size</code> would also be sensible but I didn't go there.)
</p>
<p>
<strong>Deprecations of old functions</strong>:
Finally, in addition to these new classes I have removed a bunch functions which were depreciated years ago and depreciated some more functions, as discussed on sage-combinat. I also reinstated the beta_numbers() methods which were removed in the last patch to partition.py (this patch said that beta_numbers and frobenius_coordinates are identical objects, but they are actually different).
</p>
<p>
For discussion about the functions being deprecated please see the following two discussions on sage-combinat:
</p>
<ul><li><a class="ext-link" href="https://groups.google.com/forum/?fromgroups=#!topic/sage-combinat-devel/NFNRYjqoouM"><span class="icon"></span>Implementation of partition tuples</a>
</li><li><a class="ext-link" href="https://groups.google.com/forum/?fromgroups=#!topic/sage-combinat-devel/utAsQzZQKLo"><span class="icon"></span>number_of_partitions and friends</a> and <a class="ext-link" href="https://groups.google.com/forum/?fromgroups=#!topic/sage-combinat-devel/-8yTNjkCuW0"><span class="icon"></span>number_of_partitions</a>
</li></ul><p>
Below is a summary of the above listed in order of what I think is decreasing controversy.
</p>
<ol><li>A=sage.combinat.partition.number_of_partitions() is marked for deprecation in favour of B=sage.combinat.partitions.number_of_partitions(), which is what function A() calls most of the time. As agreed above, number_of_partitions() will stay in the global name space, but this made the deprecation somewhat fiddly as I did not want to deprecate number_of_partitions() for "normal use" because from the user perspective this function will not change. Instead, I have deprecated the individual options of number_of_partitions() so deprecation warnings are only generated when A() does NOT call B(). In the global namespace, number_of_partitions still points to A(). When the functions which are marked for deprecation below are removed, number_of_partitions() should be changed to point to B() and A() should be changed into a deprecated_function_alias to B(). See the patch for more details.
</li></ol><ol start="2"><li>For use in Partitions().random_element() the function number_of_partitions() was cached. This cached function was almost never used so, assuming that caching this function is a good idea, I decided to use a cached version of number_of_partitions() inside partition.py always. <em>As shown in the comments below, this leads to a dramatic speed-up.</em>
</li></ol><blockquote>
<p>
This probably should be <strong>revisited</strong> when Fredrik Johansson's patch <a class="closed ticket" href="https://trac.sagemath.org/ticket/13199" title="enhancement: Use FLINT to compute the partition function (closed: fixed)">#13199</a>, which uses FLINT to implement a faster version of number_of_partitions, is merged into sage.
</p>
</blockquote>
<ol start="3"><li>The two functions
<ul><li>cyclic_permutations_of_partition
</li><li>cyclic_permutations_of_partition_iterator
</li></ul></li></ol><blockquote>
<p>
are deprecated in sage.combinat.partition and they have been moved to sage.combinat.set_partition and renamed ...._of_set_partition... As far as I can tell these functions are never used but, in any case, they are methods on set partitions rather than partitions. Nonetheless, they need to be deprecated from the global name space.
</p>
</blockquote>
<ol start="4"><li>The following functions were marked for deprecation several years ago so they have been removed from sage.combinat.partition.py:
<ul><li>partitions_list
</li><li>number_of_partitions_list
</li><li>partitions_restricted
</li><li>number_of_partitions_restricted
</li></ul></li></ol><ol start="5"><li>For the reasons given in <a class="closed ticket" href="https://trac.sagemath.org/ticket/5478" title="defect: [with patch, positive review] RestrictedPartitions is very slow and a ... (closed: fixed)">#5478</a>, <a class="missing wiki">RestrictedPartitions?</a> was also slated for removal but it was decided not to deprecate this class until Partitions() is able to process the appropriate combinations of keyword arguments. See <a class="new ticket" href="https://trac.sagemath.org/ticket/12278" title="defect: misleading (outdated) docstring in partitions_restricted (new)">#12278</a> and the comment by John Palmieri below for more details. Nicolas has suggested that one way of addressing this may be to refactor the partitions code so that it uses Florent's enumerated sets factories <a class="closed ticket" href="https://trac.sagemath.org/ticket/10194" title="enhancement: Set factories (closed: fixed)">#10194</a>.
</li></ol><ol start="6"><li>The following functions now give deprecation warnings and they are marked for removal from the global name space:
<ul><li>partitions_set
</li><li>number_of_partitions_set
</li><li>ordered_partitions
</li><li>number_of_ordered_partitions
</li><li>partitions,
</li><li>ferrers_diagram
</li><li>partitions_greatest
</li><li>partitions_greatest_eq
</li><li>partitions_tuples
</li><li>number_of_partitions_tuples
</li><li>partition_power
</li></ul></li></ol><p>
In all cases, these function are deprecated in favour of (methods in) parent classes.
</p>
<p>
<strong>Apply:</strong> <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13072/trac_13072-tuples-of-partitions_am.patch" title="Attachment 'trac_13072-tuples-of-partitions_am.patch' in Ticket #13072">trac_13072-tuples-of-partitions_am.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13072/trac_13072-tuples-of-partitions_am.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13072
Trac 1.1.6andrew.mathasFri, 06 Jul 2012 16:00:45 GMTstatus, description changed
https://trac.sagemath.org/ticket/13072#comment:1
https://trac.sagemath.org/ticket/13072#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=1">diff</a>)
</li>
</ul>
Ticketandrew.mathasMon, 16 Jul 2012 02:32:02 GMTwork_issues deleted
https://trac.sagemath.org/ticket/13072#comment:2
https://trac.sagemath.org/ticket/13072#comment:2
<ul>
<li><strong>work_issues</strong>
<em>Some category tests currently fails.</em> deleted
</li>
</ul>
Ticketandrew.mathasTue, 31 Jul 2012 00:08:04 GMTpriority, dependencies, description changed
https://trac.sagemath.org/ticket/13072#comment:3
https://trac.sagemath.org/ticket/13072#comment:3
<ul>
<li><strong>priority</strong>
changed from <em>minor</em> to <em>major</em>
</li>
<li><strong>dependencies</strong>
changed from <em>NA</em> to <em>#9265</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=3">diff</a>)
</li>
</ul>
<p>
Should now apply cleanly to sage 5.2
</p>
Ticketandrew.mathasWed, 01 Aug 2012 03:03:54 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:4
https://trac.sagemath.org/ticket/13072#comment:4
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=4">diff</a>)
</li>
</ul>
Ticketandrew.mathasWed, 01 Aug 2012 03:22:33 GMT
https://trac.sagemath.org/ticket/13072#comment:5
https://trac.sagemath.org/ticket/13072#comment:5
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
Ticketandrew.mathasWed, 01 Aug 2012 03:25:28 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:6
https://trac.sagemath.org/ticket/13072#comment:6
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=6">diff</a>)
</li>
</ul>
Ticketandrew.mathasFri, 03 Aug 2012 10:34:50 GMT
https://trac.sagemath.org/ticket/13072#comment:7
https://trac.sagemath.org/ticket/13072#comment:7
<p>
New version of patch which creates a new file partition_tuple.py which contains all of the partition_tuple code.
</p>
Ticketandrew.mathasTue, 14 Aug 2012 13:05:30 GMT
https://trac.sagemath.org/ticket/13072#comment:8
https://trac.sagemath.org/ticket/13072#comment:8
<p>
For the patchbot:
</p>
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
TickettscrimFri, 24 Aug 2012 19:36:47 GMTreviewer set
https://trac.sagemath.org/ticket/13072#comment:9
https://trac.sagemath.org/ticket/13072#comment:9
<ul>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
</ul>
Ticketandrew.mathasWed, 29 Aug 2012 04:51:26 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:10
https://trac.sagemath.org/ticket/13072#comment:10
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=10">diff</a>)
</li>
</ul>
Ticketandrew.mathasWed, 29 Aug 2012 04:54:06 GMT
https://trac.sagemath.org/ticket/13072#comment:11
https://trac.sagemath.org/ticket/13072#comment:11
<p>
For the patchbot:
</p>
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
Ticketandrew.mathasWed, 29 Aug 2012 04:57:15 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:12
https://trac.sagemath.org/ticket/13072#comment:12
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=12">diff</a>)
</li>
</ul>
Ticketandrew.mathasWed, 29 Aug 2012 09:26:16 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:13
https://trac.sagemath.org/ticket/13072#comment:13
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=13">diff</a>)
</li>
</ul>
Ticketandrew.mathasWed, 29 Aug 2012 12:27:43 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:14
https://trac.sagemath.org/ticket/13072#comment:14
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=14">diff</a>)
</li>
</ul>
<p>
The following timings show that there is a dramatic speed-up when number_of_partitions is cached:
</p>
<p>
<strong>With caching</strong>:
</p>
<pre class="wiki">sage: %timeit [Partitions(n).random_element() for n in range(100)]
25 loops, best of 3: 25 ms per loop
sage: %timeit [Partitions(n).random_element() for n in range(100)]
25 loops, best of 3: 24.6 ms per loop
sage: %timeit [Partitions(n).random_element() for n in range(100)]
25 loops, best of 3: 25.4 ms per loop
</pre><p>
<strong>Without caching</strong>:
</p>
<pre class="wiki">sage: %timeit [Partitions(n).random_element() for n in range(100)]
5 loops, best of 3: 1.23 s per loop
sage: %timeit [Partitions(n).random_element() for n in range(100)]
5 loops, best of 3: 1.23 s per loop
sage: %timeit [Partitions(n).random_element() for n in range(100)]
5 loops, best of 3: 1.26 s per loop
</pre>
Ticketandrew.mathasWed, 29 Aug 2012 12:37:40 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:15
https://trac.sagemath.org/ticket/13072#comment:15
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=15">diff</a>)
</li>
</ul>
TicketjhpalmieriWed, 29 Aug 2012 19:49:27 GMT
https://trac.sagemath.org/ticket/13072#comment:16
https://trac.sagemath.org/ticket/13072#comment:16
<p>
Regarding <code>RestrictedPartitions</code>: see <a class="new ticket" href="https://trac.sagemath.org/ticket/12278" title="defect: misleading (outdated) docstring in partitions_restricted (new)">#12278</a>. Should it be deprecated at all? In particular, what's the replacement for something like <code>RestrictedPartitions(5,[3,2,1], 3)</code>? The best I can come up with is
</p>
<pre class="wiki">[p for p in Partitions(5, parts_in=[3,2,1]) if len(p) == 3]
</pre><p>
but surely this isn't ideal.
</p>
Ticketandrew.mathasThu, 30 Aug 2012 07:34:38 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:17
https://trac.sagemath.org/ticket/13072#comment:17
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=17">diff</a>)
</li>
</ul>
Ticketandrew.mathasFri, 31 Aug 2012 00:38:54 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:18
https://trac.sagemath.org/ticket/13072#comment:18
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=18">diff</a>)
</li>
</ul>
Ticketandrew.mathasFri, 31 Aug 2012 05:26:58 GMT
https://trac.sagemath.org/ticket/13072#comment:19
https://trac.sagemath.org/ticket/13072#comment:19
<p>
For the patchbot:
</p>
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
Ticketandrew.mathasFri, 31 Aug 2012 08:24:05 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:20
https://trac.sagemath.org/ticket/13072#comment:20
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=20">diff</a>)
</li>
</ul>
Ticketandrew.mathasWed, 12 Sep 2012 01:00:49 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:21
https://trac.sagemath.org/ticket/13072#comment:21
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=21">diff</a>)
</li>
</ul>
Ticketandrew.mathasMon, 08 Oct 2012 03:34:04 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:22
https://trac.sagemath.org/ticket/13072#comment:22
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=22">diff</a>)
</li>
</ul>
Ticketandrew.mathasMon, 08 Oct 2012 03:34:55 GMT
https://trac.sagemath.org/ticket/13072#comment:23
https://trac.sagemath.org/ticket/13072#comment:23
<p>
For the patchbot:
</p>
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
Ticketandrew.mathasMon, 08 Oct 2012 12:08:39 GMT
https://trac.sagemath.org/ticket/13072#comment:24
https://trac.sagemath.org/ticket/13072#comment:24
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:16" title="Comment 16">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
Regarding <code>RestrictedPartitions</code>: see <a class="new ticket" href="https://trac.sagemath.org/ticket/12278" title="defect: misleading (outdated) docstring in partitions_restricted (new)">#12278</a>. Should it be deprecated at all? In particular, what's the replacement for something like <code>RestrictedPartitions(5,[3,2,1], 3)</code>? The best I can come up with is
</p>
<pre class="wiki">[p for p in Partitions(5, parts_in=[3,2,1]) if len(p) == 3]
</pre><p>
but surely this isn't ideal.
</p>
</blockquote>
<p>
This was discussed in detail on sage-combinat, eventually leading to point <a class="closed ticket" href="https://trac.sagemath.org/ticket/5" title="enhancement: full keyboard mode for notebook (closed: invalid)">#5</a> in the blurb for this ticket.
</p>
TickettscrimMon, 08 Oct 2012 21:55:45 GMTstatus, dependencies changed
https://trac.sagemath.org/ticket/13072#comment:25
https://trac.sagemath.org/ticket/13072#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#9265</em> to <em>#9265, #11446</em>
</li>
</ul>
<p>
Looks good. I've added <a class="closed ticket" href="https://trac.sagemath.org/ticket/11446" title="enhancement: compute the outline of a partition (closed: fixed)">#11446</a> as a dependency since this is based respect to <a class="closed ticket" href="https://trac.sagemath.org/ticket/11446" title="enhancement: compute the outline of a partition (closed: fixed)">#11446</a> and it's dependency <a class="closed ticket" href="https://trac.sagemath.org/ticket/11442" title="enhancement: Computes the Plancherel measure of an individual partition (closed: fixed)">#11442</a> (both slightly modify <code>partition.py</code> as well).
</p>
TickethivertMon, 08 Oct 2012 22:30:47 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:26
https://trac.sagemath.org/ticket/13072#comment:26
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hi Andrew and Travis,
</p>
<p>
Thanks both for your work. I'm hate to switch back to needs works but looking at the compiled doc, I see various small problems which should be fixed. Here are some of them:
</p>
<ul><li>Don't indent bulleted list. It adds an extra uneeded indentation (see e.g. REFERENCE vs AUTHORS in the module class;
</li></ul><ul><li>There is a proper markup for references (see developper guide);
</li></ul><ul><li>In the doc of the class <code>PartitionTuple</code>, there is a miss indentation between <code>INPUT</code> and <code>EXAMPLES:</code>
</li></ul><ul><li>in the doc of <code>Garnir_tableau</code> please write <code>``self``</code> <code>``cell``</code>, <code>``FALSE``</code> ... (verbatim set-up) but don't forget single back-quote for <code>`(k,a+1,c)`</code> (latex set-up) and similar. The hyperlink in SEE ALSO are missing
</li></ul><ul><li>There is a proper markup for linking to trac ticket eg: <code>:trac:`13123`</code>
</li></ul><ul><li>There is a typo in "The Garnir tableau are the “first” non-standard tableaux which arise when you at by simple transpositions."
</li></ul><p>
Sorry for being picky for the doc. If it wasn't so late et France, I would have written a review patch.
</p>
<p>
For Travis: please check the compiled doc when your are reviewing a patch. You can refer to <code>http://wiki.sagemath.org/ReviewChecklist</code>.
</p>
<p>
Cheers,
</p>
<p>
Florent
</p>
Ticketandrew.mathasMon, 08 Oct 2012 22:51:01 GMT
https://trac.sagemath.org/ticket/13072#comment:27
https://trac.sagemath.org/ticket/13072#comment:27
<p>
Hi Florent,
</p>
<p>
Thanks for the specific comments about what needs to be fixed. I'll go through and fix these.
</p>
<blockquote class="citation">
<p>
For Travis: please check the compiled doc when your are reviewing a patch. You can refer to <code>http://wiki.sagemath.org/ReviewChecklist</code>.
</p>
</blockquote>
<p>
Actually, Travis has already spent a large amount of time fixing my doc strings, so there would be many more problems without the large amount of time that he has already spent on this.
</p>
<p>
Andrew
</p>
Ticketandrew.mathasMon, 08 Oct 2012 23:33:02 GMT
https://trac.sagemath.org/ticket/13072#comment:28
https://trac.sagemath.org/ticket/13072#comment:28
<p>
Hi Florent,
</p>
<p>
I think that the revised patch fixes all of the problems that you highlighted -- well, except for the Garnir tableaux issues which are in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13074" title="enhancement: Implementation of TableauTuples (closed: fixed)">#13074</a>.
</p>
<p>
Cheers,
Andrew
</p>
TickethivertTue, 09 Oct 2012 05:50:55 GMT
https://trac.sagemath.org/ticket/13072#comment:29
https://trac.sagemath.org/ticket/13072#comment:29
<blockquote>
<p>
Hi Andrew,
</p>
</blockquote>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:27" title="Comment 27">andrew.mathas</a>:
</p>
<blockquote class="citation">
<p>
Actually, Travis has already spent a large amount of time fixing my doc strings, so there would be many more problems without the large amount of time that he has already spent on this.
</p>
</blockquote>
<p>
Sorry, I you feel I'm being rude. It wasn't my intend. However, I've no chance to know that since there is no record of his work on this ticket. Even when the review is done offline, it is good to keep a (not necessarily detailed) trace of what has been done on the ticket, if only to give proper credit.
</p>
<p>
Cheers,
</p>
<p>
Florent
</p>
<p>
PS: Unfortunately, I've no time to answer your comment on <a class="closed ticket" href="https://trac.sagemath.org/ticket/13074" title="enhancement: Implementation of TableauTuples (closed: fixed)">#13074</a>, right now (run to catch a train + teaching).
</p>
Ticketandrew.mathasTue, 09 Oct 2012 08:02:54 GMT
https://trac.sagemath.org/ticket/13072#comment:30
https://trac.sagemath.org/ticket/13072#comment:30
<p>
Hi Florent,
</p>
<p>
No, no, that's OK. I just wanted to acknowledge that Travis has done a lot of work on the documentation and that any errors that remained were mine and mine alone. I appreciate your looking at the patch and trying to improve it.
</p>
<p>
I have mostly sorted out the doc string problems in these two patches but there are till one or two that remain. If there is any trick to working out where the errors appear in the code from the rest errors messages please let me know when you have the time as currently I am doing pseudo random searches.
</p>
<p>
Thanks again,
Andrew
</p>
Ticketandrew.mathasTue, 09 Oct 2012 13:18:28 GMT
https://trac.sagemath.org/ticket/13072#comment:31
https://trac.sagemath.org/ticket/13072#comment:31
<p>
I think that all of the doc string issues really are fixed now.
</p>
TickettscrimTue, 09 Oct 2012 15:34:51 GMT
https://trac.sagemath.org/ticket/13072#comment:32
https://trac.sagemath.org/ticket/13072#comment:32
<p>
Florent,
</p>
<p>
Thank you for catching these docstring problems. I appreciate you begin picky about the docs. I didn't know about the indentation of the bullet lists, and I did miss that indentation of EXAMPLES:: block in the compiled doc.
</p>
<p>
Andrew,
</p>
<p>
A few more minor issues.
</p>
<ul><li>I believe this line (line 263 in <code>partition_tuple.py</code>) has a comma misplaced:
<pre class="wiki">When these algebras are not semisimple partition, tuples index...
</pre></li><li>In <code>partition_tuple.hook_length()</code> it should be <code>coordinates of the form `(k,r,c)`</code>
</li><li>In <code>partition.random_element()</code>, the inputs <code>uniform</code> and <code>Plancherel</code> should be indented one more since they are the types of inputs for <code>measure</code>.
</li></ul>
Ticketandrew.mathasWed, 10 Oct 2012 03:23:41 GMT
https://trac.sagemath.org/ticket/13072#comment:33
https://trac.sagemath.org/ticket/13072#comment:33
<p>
Hi Travis,
</p>
<p>
I have uploaded a new version of the patch which fixes the errant comma and the indentation in random_element but I think that the <code>``(k,r,c)``</code> in the hook_length function is correct because k,r and c are arguments to this function. Of course, it would be more correct to write <code>(``k``,``r``,``c``)</code> but ReST complains about this. I guess that <code>(``k``, ``r``, ``c``)</code> might be OK, although I suspect that it will want a few more spaces like <code>( ``k`` , ``r`` , ``c`` )</code> to be legal syntax. I prefer using <code>``(k,r,c)``</code> as these three integers together comprise a cell which is really a "single" variable... Let me know if you disagree.
</p>
<p>
Cheers,
Andrew
</p>
TickettscrimWed, 10 Oct 2012 15:15:03 GMT
https://trac.sagemath.org/ticket/13072#comment:34
https://trac.sagemath.org/ticket/13072#comment:34
<p>
Hey Andrew,
</p>
<p>
Yes, it should be <code>``(k,r,c)``</code> but right now it is <code>``(r,c)``</code>. Although on further thought, it might be better to move the note about 0-based (and adding <code>``k``</code>) and the python *-operator to the header. That way it covers all of the similar functions.
</p>
<p>
Thanks,
Travis
</p>
Ticketandrew.mathasFri, 12 Oct 2012 03:45:06 GMT
https://trac.sagemath.org/ticket/13072#comment:35
https://trac.sagemath.org/ticket/13072#comment:35
<p>
Thanks Travis, you are right of course. I've updated the patch.
</p>
<p>
Andrew
</p>
<p>
--
</p>
<p>
For the patchbot:
</p>
<p>
Apply trac_13072-tuples-of-partitions_am.patch
</p>
TickettscrimFri, 12 Oct 2012 04:00:59 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:36
https://trac.sagemath.org/ticket/13072#comment:36
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
Thanks Andrew! Looks good. I like the notes.
</p>
TicketjdemeyerFri, 12 Oct 2012 14:10:01 GMTmilestone changed
https://trac.sagemath.org/ticket/13072#comment:37
https://trac.sagemath.org/ticket/13072#comment:37
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.4</em> to <em>sage-5.5</em>
</li>
</ul>
TicketjdemeyerSat, 13 Oct 2012 10:14:34 GMTdescription changed
https://trac.sagemath.org/ticket/13072#comment:38
https://trac.sagemath.org/ticket/13072#comment:38
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13072?action=diff&version=38">diff</a>)
</li>
</ul>
TicketjdemeyerSat, 13 Oct 2012 21:37:18 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:39
https://trac.sagemath.org/ticket/13072#comment:39
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<pre class="wiki">sage -t -force_lib devel/sage/sage/structure/sage_object.pyx
**********************************************************************
File "/release/merger/sage-5.5.beta0/devel/sage-main/sage/structure/sage_object.pyx", line 1114:
sage: sage.structure.sage_object.unpickle_all() # (4s on sage.math, 2011)
Expected:
doctest:... DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
See http://trac.sagemath.org/4260 for details.
Successfully unpickled ... objects.
Failed to unpickle 0 objects.
Got:
* unpickle failure: load('/release/merger/sage-5.5.beta0/home/.sage/tmp/sage.math.washington.edu/5737/dir_i3wI40//pickle_jar/_class__sage_combinat_partition_PartitionTuples_nk__.sobj')
doctest:1172: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
See http://trac.sagemath.org/4260 for details.
Failed:
_class__sage_combinat_partition_PartitionTuples_nk__.sobj
Successfully unpickled 593 objects.
Failed to unpickle 1 objects.
**********************************************************************
</pre>
TicketjdemeyerSat, 13 Oct 2012 21:38:00 GMT
https://trac.sagemath.org/ticket/13072#comment:40
https://trac.sagemath.org/ticket/13072#comment:40
<pre class="wiki">sage -t --long -force_lib devel/sage/sage/combinat/partition.py
**********************************************************************
File "/release/merger/sage-5.5.beta0/devel/sage-main/sage/combinat/partition.py", line 434:
sage: all(test2(core,tuple(mus)) # long time (5s on sage.math, 2011)
for k in range(Integer(1),Integer(10))
for n_core in range(Integer(10)-k)
for core in Partitions(n_core)
if core.core(k) == core
for n_mus in range(Integer(10)-k)
for mus in PartitionTuples(n_mus,k))
Exception raised:
Traceback (most recent call last):
File "/release/merger/sage-5.5.beta0/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/release/merger/sage-5.5.beta0/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/release/merger/sage-5.5.beta0/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_5[6]>", line 2, in <module>
for k in range(Integer(1),Integer(10))
File "<doctest __main__.example_5[6]>", line 7, in <genexpr>
for mus in PartitionTuples(n_mus,k))
File "classcall_metaclass.pyx", line 279, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (sage/misc/classcall_metaclas
s.c:946)
File "/release/merger/sage-5.5.beta0/local/lib/python/site-packages/sage/combinat/partition_tuple.py", line 1059, in __classcall_pri
vate__
raise ValueError, 'the level must be a positive integer'
ValueError: the level must be a positive integer
**********************************************************************
</pre>
Ticketandrew.mathasSun, 14 Oct 2012 13:44:15 GMT
https://trac.sagemath.org/ticket/13072#comment:41
https://trac.sagemath.org/ticket/13072#comment:41
<p>
Hi Jeroen,
</p>
<p>
Perhaps I am confused, but most of these pickles shouldn't exist any more as they should have been removed by the new pickle jar attached to <a class="closed ticket" href="https://trac.sagemath.org/ticket/9265" title="enhancement: Remove `CombinatorialClass` from sage.combinat.tableau (closed: fixed)">#9265</a>. Specifically, the pickles
</p>
<blockquote>
<p>
_class__ sage_combinat_skew_tableau_SemistandardSkewTableaux_n__ .sobj')<br /> _class__ sage_combinat_skew_tableau_SemistandardSkewTableaux_nmu__ .sobj')<br /> _class__ sage_combinat_skew_tableau_SemistandardSkewTableaux_p__ .sobj')<br /> _class__ sage_combinat_skew_tableau_SemistandardSkewTableaux_pmu__ .sobj')<br /> _class__ sage_combinat_skew_tableau_StandardSkewTableaux_n__ .sobj')<br /> _class__ sage_combinat_tableau_SemistandardTableaux_n__ .sobj')<br /> _class__ sage_combinat_tableau_SemistandardTableaux_nmu__ .sobj')<br /> _class__ sage_combinat_tableau_SemistandardTableaux_p__ .sobj')<br /> _class__ sage_combinat_tableau_SemistandardTableaux_pmu__ .sobj')<br /> _class__ sage_combinat_tableau_StandardTableaux_n__ .sobj')<br /> _class__ sage_combinat_tableau_StandardTableaux_partition__ .sobj')<br /> _class__ sage_combinat_tableau_Tableau_class__ .sobj')<br /> _class__ sage_combinat_tableau_Tableaux_n__ .sobj')
</p>
</blockquote>
<p>
shouldn't be there having been replaced with new improved pickles with slightly more informative names (for example, _n_ --> _size_, _p_ --> _shape_ etc.). The pickle
</p>
<blockquote>
<p>
_class__ sage_combinat_partition_PartitionTuples_nk__ .sobj
</p>
</blockquote>
<p>
I agree is mine to fix but I am also not entirely convinced that the first three pickles are caused by this patch, that is the following pickles:
</p>
<blockquote>
<p>
class__ sage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class__ .sobj')<br /> class__ sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class__ .sobj')<br /> class__ sage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category__ .sobj')
</p>
</blockquote>
<p>
as I think that I might have also created new pickles for these in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9265" title="enhancement: Remove `CombinatorialClass` from sage.combinat.tableau (closed: fixed)">#9265</a>. I am, of course, happy to rebuild them just in case.
</p>
<p>
Can you please confirm that the pickle_jar was updated as per the attachment for <a class="closed ticket" href="https://trac.sagemath.org/ticket/9265" title="enhancement: Remove `CombinatorialClass` from sage.combinat.tableau (closed: fixed)">#9265</a>. The mistake is quite probably mine as I assumed that the whole pickle jar would be replaced whereas if you just added in the new pickles then you would not have been aware that some of the old pickles needed to be deleted (although presumably it would not have been possible to unpickle them...??). If there is any documentation on updating the pickle jar please let me know.
</p>
<p>
Please advise.
</p>
<p>
Cheers,
</p>
<p>
Andrew<br />
</p>
TicketjdemeyerMon, 15 Oct 2012 08:03:29 GMT
https://trac.sagemath.org/ticket/13072#comment:42
https://trac.sagemath.org/ticket/13072#comment:42
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:41" title="Comment 41">andrew.mathas</a>:
</p>
<blockquote class="citation">
<p>
Can you please confirm that the pickle_jar was updated as per the attachment for <a class="closed ticket" href="https://trac.sagemath.org/ticket/9265" title="enhancement: Remove `CombinatorialClass` from sage.combinat.tableau (closed: fixed)">#9265</a>.
</p>
</blockquote>
<p>
Yes, certainly it was updated.
</p>
TicketjdemeyerMon, 15 Oct 2012 08:05:21 GMT
https://trac.sagemath.org/ticket/13072#comment:43
https://trac.sagemath.org/ticket/13072#comment:43
<p>
I don't quite understand what you're saying, since my failed test is only complaining about <strong>one pickle</strong>, namely
</p>
<pre class="wiki">_class__sage_combinat_partition_PartitionTuples_nk__.sobj
</pre>
Ticketandrew.mathasMon, 15 Oct 2012 09:08:19 GMT
https://trac.sagemath.org/ticket/13072#comment:44
https://trac.sagemath.org/ticket/13072#comment:44
<p>
Sorry, my mistake: I was testing on 5.3.<br /><br />A.
</p>
Ticketandrew.mathasMon, 15 Oct 2012 13:29:53 GMT
https://trac.sagemath.org/ticket/13072#comment:45
https://trac.sagemath.org/ticket/13072#comment:45
<p>
OK, I have fixed the long-test problem. With the pickle it seems to me that the only way to fix the problem is to replace the bad
</p>
<pre class="wiki">PartitionTuples_nk
</pre><p>
pickle in the pickle jar with a good one (and leave the other pickles untouched) as the underlying class has changed too much (vbraun posted a comment on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9265" title="enhancement: Remove `CombinatorialClass` from sage.combinat.tableau (closed: fixed)">#9265</a> suggesting that I should use <code>register_unpickle_override instead but I tried experimenting with this and it doesn't seem to work).</code>
</p>
<p>
<br />Jeroen can you please confirm that you are are happy for me to do this.<br /><br />
</p>
TicketjdemeyerMon, 15 Oct 2012 13:40:52 GMT
https://trac.sagemath.org/ticket/13072#comment:46
https://trac.sagemath.org/ticket/13072#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:45" title="Comment 45">andrew.mathas</a>:
</p>
<blockquote class="citation">
<p>
Jeroen can you please confirm that you are are happy for me to do this.
</p>
</blockquote>
<p>
Given the comments of Volker Braun and Simon King, I cannot be happy with this. No objects should be removed from the pickle jar.
</p>
TicketnthieryMon, 15 Oct 2012 13:53:15 GMT
https://trac.sagemath.org/ticket/13072#comment:47
https://trac.sagemath.org/ticket/13072#comment:47
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:46" title="Comment 46">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:45" title="Comment 45">andrew.mathas</a>:
</p>
<blockquote class="citation">
<p>
Jeroen can you please confirm that you are are happy for me to do this.
</p>
</blockquote>
<p>
Given the comments of Volker Braun and Simon King, I cannot be happy with this. No objects should be removed from the pickle jar.
</p>
</blockquote>
<p>
Just my 2 cents: at this point, partition tuples are a rather peripheral feature. If anyone has saved some pickle containing one, most likely he is in the Sage-Combinat group. Well most likely it's Andrew actually. So I vote for not wasting Andrew's time and simply dropping backward compatibility in that particular situation. I am not taking much risk by volunteering to help whoever might have trouble with such an old pickle :-)
</p>
<p>
Of course, the official procedure would be to run a poll; if you insist, we can do that on Sage-Combinat devel.
</p>
TicketjdemeyerMon, 15 Oct 2012 14:47:03 GMT
https://trac.sagemath.org/ticket/13072#comment:48
https://trac.sagemath.org/ticket/13072#comment:48
<p>
Nicolas, I started a thread on sage-devel about the pickle jar.
</p>
Ticketandrew.mathasWed, 17 Oct 2012 12:21:28 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:49
https://trac.sagemath.org/ticket/13072#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Unpickling works now.
</p>
TickettscrimFri, 19 Oct 2012 22:30:06 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:50
https://trac.sagemath.org/ticket/13072#comment:50
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Everything looks good to me (double checked the pickle jar and <code>sage_object.pyx</code>).
</p>
TicketjdemeyerThu, 01 Nov 2012 10:19:27 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:51
https://trac.sagemath.org/ticket/13072#comment:51
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Removing all whitespace everywhere is a bad idea, don't do it as it will lead to merge conflicts (unless it was approved by the sage-combinat group, then I take back my words).
</p>
Ticketandrew.mathasThu, 01 Nov 2012 11:35:02 GMT
https://trac.sagemath.org/ticket/13072#comment:52
https://trac.sagemath.org/ticket/13072#comment:52
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13072#comment:51" title="Comment 51">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Removing all whitespace everywhere is a bad idea, don't do it as it will lead to merge conflicts (unless it was approved by the sage-combinat group, then I take back my words).
</p>
</blockquote>
<p>
Jeroen, I removed whitespaces added by the patch and then checked that the sage-combiat queue applied cleanly before pushing the patch both to the sage-combinat queue and back to trac. I did not remove all white space from the source files affected by this patch as, I agree, this would probably cause havoc with the queue. As all of the patches in the sage-combinat queue still apply cleanly I am putting this back to a positive review.
</p>
Ticketandrew.mathasThu, 01 Nov 2012 11:35:21 GMTstatus changed
https://trac.sagemath.org/ticket/13072#comment:53
https://trac.sagemath.org/ticket/13072#comment:53
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
TicketjdemeyerThu, 01 Nov 2012 12:04:22 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/13072#comment:54
https://trac.sagemath.org/ticket/13072#comment:54
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.5.beta1</em>
</li>
</ul>
TicketjdemeyerThu, 01 Nov 2012 12:08:58 GMT
https://trac.sagemath.org/ticket/13072#comment:55
https://trac.sagemath.org/ticket/13072#comment:55
<p>
The new patch needs a proper commit message.
</p>
Ticketandrew.mathasThu, 01 Nov 2012 12:28:10 GMTattachment set
https://trac.sagemath.org/ticket/13072
https://trac.sagemath.org/ticket/13072
<ul>
<li><strong>attachment</strong>
set to <em>trac_13072-tuples-of-partitions_am.patch</em>
</li>
</ul>
<p>
Adding proper commit message
</p>
Ticket