Opened 7 months ago

Closed 7 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) Commit: be076bb7408723048fab91ef388b4c9daf540055
Dependencies: Stopgaps:

Description

Fix combinat.set_partition doctests for python.

Change History (20)

comment:1 Changed 7 months ago by vklein

  • Branch set to u/vklein/27777

comment:2 Changed 7 months ago by vklein

  • Commit set to 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed
  • Status changed from new to needs_review

New commits:

633e74fTrac 27777: Fix set_partition doctests for py3

comment:3 follow-up: Changed 7 months ago by tscrim

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 7 months ago by tscrim

  • 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 7 months ago by vklein

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]]
Last edited 7 months ago by vklein (previous) (diff)

comment:6 in reply to: ↑ 3 Changed 7 months ago by vklein

Replying to tscrim:

as the enumeration of set partitions should be constant.

Why ? The sames elements are present in both lists.

comment:7 Changed 7 months ago by tscrim

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 7 months ago by git

  • Commit changed from 633e74f3818c85cdd4acd16ceb2e16c3fcd97aed to cf38ee00028f49a81b2e49f54d1f2b264de5533e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5485a43Trac 27777: Fix set_partition doctests for py3
4ddc4c9Trac #27777: Sort SetPartitions_setparts._set ...
cf38ee0Trac #27777: Fix SetPartition _latex_ doctest ...

comment:9 Changed 7 months ago by vklein

  • 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 7 months ago by tscrim

One last little thing, you should not have a bare except: statement.

comment:11 Changed 7 months ago by git

  • Commit changed from cf38ee00028f49a81b2e49f54d1f2b264de5533e to db9485aaf703748845fb2946c65d5147b1abb0b2

Branch pushed to git repo; I updated commit sha1. New commits:

db9485aTrac #27777: change `except:` statement into ...

comment:12 Changed 7 months ago by vklein

Yes my mistake. It's fixed.

comment:13 Changed 7 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:14 Changed 7 months ago by mantepse

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 7 months ago by fbissey

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 7 months ago by vklein

  • Status changed from positive_review to needs_work

Indeed it looks like it's a side effect of this ticket

comment:17 Changed 7 months ago by git

  • Commit changed from db9485aaf703748845fb2946c65d5147b1abb0b2 to be076bb7408723048fab91ef388b4c9daf540055

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2b482e9Trac #27777: Fix set_partition doctests for py3
40080d5Trac #27777: Sort SetPartitions_setparts._set ...
549d21fTrac #27777: Fix SetPartition _latex_ doctest ...
fa04e75Trac #27777: change `except:` statement into ...
be076bbTrac #27777: Fix the symgp_conjugacy_class.py ...

comment:18 Changed 7 months ago by vklein

  • 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 7 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:20 Changed 7 months ago by vbraun

  • Branch changed from u/vklein/27777 to be076bb7408723048fab91ef388b4c9daf540055
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.