Opened 22 months ago
Closed 22 months ago
#27777 closed enhancement (fixed)
Py3: Fix combinat.set_partition doctests
Reported by: | vklein | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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 22 months ago by
- Branch set to u/vklein/27777
comment:2 Changed 22 months ago by
- Commit set to 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed
- Status changed from new to needs_review
comment:3 follow-up: ↓ 6 Changed 22 months 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 22 months 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 22 months 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.
Enumerating the linear extensions give me the same orders in py2 and py3:
sage: P = sp._set_partition_poset() sage: [e for e in P.linear_extensions()] [[0, 1, 2], [1, 2, 0], [1, 0, 2]]
comment:6 in reply to: ↑ 3 Changed 22 months 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 22 months 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 22 months ago by
- Commit changed from 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed to cf38ee00028f49a81b2e49f54d1f2b264de5533e
comment:9 Changed 22 months 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 22 months ago by
One last little thing, you should not have a bare except:
statement.
comment:11 Changed 22 months 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 22 months ago by
Yes my mistake. It's fixed.
comment:13 Changed 22 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:14 Changed 22 months 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 non-sortable self._set
is supported)
comment:15 Changed 22 months 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.0-167-generic/arando/2019-05-22%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 22 months 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 22 months 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 22 months 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 22 months ago by
- Status changed from needs_review to positive_review
comment:20 Changed 22 months 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