Opened 5 years ago
Closed 4 years ago
#19488 closed defect (fixed)
Random failure in AffineCrystalFromClassicalElement.__cmp__
Reported by:  vbraun  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  combinatorics  Keywords:  random_fail 
Cc:  tscrim  Merged in:  
Authors:  Frédéric Chapoton, Travis Scrimshaw  Reviewers:  Travis Scrimshaw, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  ebcd19f (Commits)  Commit:  ebcd19f0d1d8f8f2f89ed22666143a7f3ea5de15 
Dependencies:  Stopgaps: 
Description
Random failure, here on OSX:
sage t long src/sage/combinat/crystals/affine.py ********************************************************************** File "src/sage/combinat/crystals/affine.py", line 574, in sage.combinat.crystals.affine.AffineCrystalFromClassicalElement.__cmp__ Failed example: cmp(b,1) Expected: 1 Got: 1 ********************************************************************** 1 item had failures: 1 of 7 in sage.combinat.crystals.affine.AffineCrystalFromClassicalElement.__cmp__
Change History (23)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
 Branch set to u/chapoton/19488
 Commit set to 1113e99f2d62cffa4c6dfc5064e52322ad416567
comment:3 Changed 5 years ago by
I am pretty sure this is not correct:
if parent(self) is not parent(other): return NotImplemented
You need to check if the comparison is !=
as otherwise you would have 1 == b
and 1 != b
both return False
.
Could you also elaborate more on what you mean by this?
I am a bit disappointed by the results of comparison with op in
<,>,>=,<=
.
comment:4 Changed 5 years ago by
 Commit changed from 1113e99f2d62cffa4c6dfc5064e52322ad416567 to 40ede9e3bebe42618af46ce06163b65f89d5f052
Branch pushed to git repo; I updated commit sha1. New commits:
40ede9e  trac 19488 using richcmp via coercion

comment:5 Changed 5 years ago by
I have replace __richcmp__
by _richcmp_
, for which one can assume that
arguments have the same parents. Now one gets
sage: b==1 False sage: b!=1 True
But
sage: cmp(b,c) 1 sage: cmp(c,b) 1 sage: c<b False sage: b<c False
Maybe for coherence we will need to apply the same treatment to all crystals ?
comment:6 Changed 5 years ago by
 Milestone changed from sage6.10 to sage7.4
comment:7 Changed 5 years ago by
You forget that if cmp does not error out, then it forces some total ordering (probably memory location), so that behavior is consistent with this. I don't think at this point we need to doing anything elsewhere.
comment:8 Changed 5 years ago by
This is really puzzling to me (on this branch):
sage: b<c False sage: cmp(b,c) 1 sage: b._richcmp_(c,0) True sage: b.__cmp__(c) 1
what is the first line doing, if not calling either cmp or __cmp__
or rich comparison ?
comment:9 Changed 5 years ago by
This branch triggers failing doctests in other crystals, as I was expecting:
sage t long src/sage/categories/regular_crystals.py # 3 doctests failed sage t long src/sage/combinat/crystals/subcrystal.py # 6 doctests failed
comment:10 Changed 5 years ago by
 Commit changed from 40ede9e3bebe42618af46ce06163b65f89d5f052 to 18307b3fbdb825fee35e3cb0e492dfeac157bfa1
comment:11 Changed 5 years ago by
 Branch changed from u/chapoton/19488 to public/crystals/remove_cmp_crystals19488
 Commit changed from 18307b3fbdb825fee35e3cb0e492dfeac157bfa1 to 6f36d1a0fc47848ffc083faae69f9f46df75713e
 Reviewers set to Travis Scrimshaw, Frédéric Chapoton
 Status changed from new to needs_review
I traced the problem down to ElementWrapper
defining a __richcmp__
. So I changed that to _richcmp_
and fixed a few trivial doctest failures. Now off to the patchbot.
New commits:
6f36d1a  Changing ElementWrapper.__richcmp__ to _richcmp_.

comment:12 Changed 5 years ago by
 Commit changed from 6f36d1a0fc47848ffc083faae69f9f46df75713e to 7f8d438404a118b99c92238e4da5bedf494e0f81
Branch pushed to git repo; I updated commit sha1. New commits:
7f8d438  trac 19488 fixing some of the failing doctests

comment:13 Changed 5 years ago by
I modified 3 failing doctests, there just remains one in pickling.
comment:14 Changed 5 years ago by
So the problem with the pickling and _richcmp_
has to do with equality versus identical parents. The parents are equal but not identical. This causes problems for __richcmp__
, where it relies on parents being identical. It then creates an element wrapping an element of the test parent.
We can either implement the following the test parent to fix the doctest:
def _element_constructor_(self, x): if isinstance(x, ElementWrapper): if x.parent() == self: return self.element_class(self, x.value) return Parent._element_constructor_(self, x) def _an_element_(self): return self.element_class(self, "_an_element_")
The other way is to override __richcmp__
for ElementWrapper
to handle when parents are equal but not identical.
I'm thinking the latter option is the way to go forward as it is backwards compatible.
comment:15 Changed 4 years ago by
I am sorry, but I am not quite able to follow your reasoning. If you feel you have a reasonable solution, please proceed.
comment:16 Changed 4 years ago by
The problem is
sage: TestParent4() == TestParent4() True sage: TestParent4() is TestParent4()
So picking creates a new equalbutnotidentical parent. Thus there is a canonical coercion map that calls the _element_constructor_
, which simply wraps the element (the result is a wrapped element of a wrapped element). If the parents were identical, then no map would be applied (this is a shortcut of the coercion model), and so the values would simply be compared as normal.
TL;DR, yes, I have a very reasonable solution.
comment:17 Changed 4 years ago by
 Commit changed from 7f8d438404a118b99c92238e4da5bedf494e0f81 to ebcd19f0d1d8f8f2f89ed22666143a7f3ea5de15
Branch pushed to git repo; I updated commit sha1. New commits:
ebcd19f  Reverting to previous behavior of allowing comparison of equal parents.

comment:18 Changed 4 years ago by
patchbot is green.
comment:19 followup: ↓ 20 Changed 4 years ago by
Just to be sure, __richcmp__
will be called first, is this right ?
And why do these two richcmp methods do not use self as first parameter ?
comment:20 in reply to: ↑ 19 Changed 4 years ago by
Replying to chapoton:
Just to be sure,
__richcmp__
will be called first, is this right ?
Yes. __richcmp__
is called by Python, whereas _richcmp_
is the Sage version that is called from __richcmp__
.
And why do these two richcmp methods do not use self as first parameter ?
So self
is known to Cython to be an ElementWrapper
for speed for _richcmp_
(or so I've been told). For __richcmp__
, it was a result of copy/paste and laziness. I'm okay with changing both of them.
comment:21 Changed 4 years ago by
 Status changed from needs_review to positive_review
oh, well. I am happy enough with the current state, let us move to something else.
Do you think we should give the same treatment to other crystals ?
comment:22 Changed 4 years ago by
We can if you want, but it will be a low priority for me for the next week.
comment:23 Changed 4 years ago by
 Branch changed from public/crystals/remove_cmp_crystals19488 to ebcd19f0d1d8f8f2f89ed22666143a7f3ea5de15
 Resolution set to fixed
 Status changed from positive_review to closed
This looks like something that is going to be introduced by the
__hash__
people on #19016. This doctest does not exist currently in Sage.