Opened 2 years ago
Closed 2 years ago
#27777 closed enhancement (fixed)
Py3: Fix combinat.set_partition doctests
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  python3  Keywords:  thursdaysbdx 
Cc:  Merged in:  
Authors:  Vincent Klein  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  be076bb (Commits, GitHub, GitLab)  Commit:  be076bb7408723048fab91ef388b4c9daf540055 
Dependencies:  Stopgaps: 
Description
Fix combinat.set_partition doctests for python.
Change History (20)
comment:1 Changed 2 years ago by
 Branch set to u/vklein/27777
comment:2 Changed 2 years ago by
 Commit set to 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed
 Status changed from new to needs_review
comment:3 followup: ↓ 6 Changed 2 years ago by
This change worries me:
 sage: SetPartitions(["a", "b", "c"], [2,1]).list()  [{{'a'}, {'b', 'c'}}, {{'a', 'c'}, {'b'}}, {{'a', 'b'}, {'c'}}] + sage: sorted(SetPartitions(["a", "b", "c"], [2,1]), key=str) + [{{'a', 'b'}, {'c'}}, {{'a', 'c'}, {'b'}}, {{'a'}, {'b', 'c'}}]
as the enumeration of set partitions should be constant. Maybe something is slightly different in the algorithm, but this needs a closer examination (that I do not have time for tonight).
comment:4 Changed 2 years ago by
 Status changed from needs_review to needs_work
Okay, so the issue should be in enumerating the linear extensions of a poset. I am not sure what to do about that, but I think we should not sort the output here as it is just a symptom of an underlying issue.
I am also getting a failure in the _latex_
method, but that also should not happen. My guess is the underlying (frozen)sets are not ordered the same way, which falls out from applying the str
.
comment:5 Changed 2 years ago by
In this case it looks like the difference of order depend on that :
py2
sage: sp = SetPartitions(["a", "b", "c"], [2,1]) sage: sp._set frozenset({'a', 'b', 'c'}) sage: list(sp._set) ['a', 'c', 'b']
py3
sage: sp = SetPartitions(["a", "b", "c"], [2,1]) sage: sp._set frozenset({'a', 'b', 'c'}) sage: list(sp._set) ['a', 'b', 'c']
The position of elements of list(sp._set)
is used for generating SetPartitions_setparts.__iter__
results.
comment:6 in reply to: ↑ 3 Changed 2 years ago by
Replying to tscrim:
as the enumeration of set partitions should be constant.
Why ? The sames elements are present in both lists.
comment:7 Changed 2 years ago by
That is a good catch, I missed that. It is also good to know that enumerating the linear extensions is the same. So that is where we need to fix things. In the __iter__
:
s = list(self._set) +try: + s = sorted(self._set) +except TypeError: + s = sorted(self._set, key=str)
It should be consistent because it is an enumerated set, so the unranking should be compatible. Imagine you run something in parallel, get the i
th element in the worker process, and then want to use the i
th element in the main process. Being an enumerated set guarantees that this is consistent. (Another scenario is replace process by sessions.)
comment:8 Changed 2 years ago by
 Commit changed from 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed to cf38ee00028f49a81b2e49f54d1f2b264de5533e
comment:9 Changed 2 years ago by
 Status changed from needs_work to needs_review
@tscrim Thanks for your help and the explanation.
SetPartitions_setparts.__iter__
has been fixed. The
_latex_
failure, which used to appear randomly, is fixed.  Branch has been rebased on 8.8.beta5.
comment:10 Changed 2 years ago by
One last little thing, you should not have a bare except:
statement.
comment:11 Changed 2 years ago by
 Commit changed from cf38ee00028f49a81b2e49f54d1f2b264de5533e to db9485aaf703748845fb2946c65d5147b1abb0b2
Branch pushed to git repo; I updated commit sha1. New commits:
db9485a  Trac #27777: change `except:` statement into ...

comment:12 Changed 2 years ago by
Yes my mistake. It's fixed.
comment:13 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you. LGTM.
comment:14 Changed 2 years ago by
self._set
is sorted in several places in this module, wouldn't it be good to make this uniform? (it is not clear to me to what extent a nonsortable self._set
is supported)
comment:15 Changed 2 years ago by
I am getting this failure which is also in the buildbot https://patchbot.sagemath.org/log/27777/Ubuntu/14.04/i686/3.13.0167generic/arando/20190522%2012:12:39?short
sage t long src/sage/groups/perm_gps/symgp_conjugacy_class.py ********************************************************************** File "src/sage/groups/perm_gps/symgp_conjugacy_class.py", line 345, in sage.groups.perm_gps.symgp_conjugacy_class.conjugacy_class_iterator Failed example: next(it) Expected: [('a', 'c'), ('b', 'e'), ('d', 'f')] Got: [('a', 'b'), ('c', 'd'), ('e', 'f')] **********************************************************************
comment:16 Changed 2 years ago by
 Status changed from positive_review to needs_work
Indeed it looks like it's a side effect of this ticket
comment:17 Changed 2 years ago by
 Commit changed from db9485aaf703748845fb2946c65d5147b1abb0b2 to be076bb7408723048fab91ef388b4c9daf540055
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2b482e9  Trac #27777: Fix set_partition doctests for py3

40080d5  Trac #27777: Sort SetPartitions_setparts._set ...

549d21f  Trac #27777: Fix SetPartition _latex_ doctest ...

fa04e75  Trac #27777: change `except:` statement into ...

be076bb  Trac #27777: Fix the symgp_conjugacy_class.py ...

comment:18 Changed 2 years ago by
 Keywords thursdaysbdx added
 Status changed from needs_work to needs_review
Rebase ticket on 8.8.beta6. Fix doctest in sympgp_conjugacy_class.py.
comment:19 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:20 Changed 2 years ago by
 Branch changed from u/vklein/27777 to be076bb7408723048fab91ef388b4c9daf540055
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 27777: Fix set_partition doctests for py3