Random failure in AffineCrystalFromClassicalElement.__cmp__

[vbraun]

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__

CC: @tscrim

Component: combinatorics

Keywords: random_fail

Author: Frédéric Chapoton, Travis Scrimshaw

Branch/Commit: ebcd19f

Reviewer: Travis Scrimshaw, Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/19488

[tscrim]

comment:1

This looks like something that is going to be introduced by the __hash__ people on #19016. This doctest does not exist currently in Sage.

[fchapoton]

Commit: 1113e99

[fchapoton]

comment:2

I tried to make a branch using the brand new possibilities of "richcmp(x,y,op)". (see #21128)

I am a bit disappointed by the results of comparison with op in <,>,>=,<=.

Travis, what do you think ?

---

New commits:

1113e99

trac 19488 converting cmp to richcmp in affine crystal

[fchapoton]

Branch: u/chapoton/19488

[tscrim]

comment:3

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 <,>,>=,<=.

[sagetrac-git]

Changed commit from 1113e99 to 40ede9e

[sagetrac-git]

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

40ede9e

trac 19488 using richcmp via coercion

[fchapoton]

comment:5

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 ?

[tscrim]

comment:7

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.

[fchapoton]

comment:8

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 ?

[fchapoton]

comment:9

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

[sagetrac-git]

Changed commit from 40ede9e to 18307b3

[sagetrac-git]

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

caa8475	Merge branch 'u/chapoton/19488' in 7.4.b2
18307b3	trac 19488 more changes in cmp for subcrystals

[tscrim]

Changed commit from 18307b3 to 6f36d1a

[tscrim]

Author: Frédéric Chapoton, Travis Scrimshaw

[tscrim]

Changed branch from u/chapoton/19488 to public/crystals/remove_cmp_crystals-19488

[tscrim]

comment:11

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_.

[tscrim]

Reviewer: Travis Scrimshaw, Frédéric Chapoton

[sagetrac-git]

Changed commit from 6f36d1a to 7f8d438

[sagetrac-git]

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

7f8d438

trac 19488 fixing some of the failing doctests

[fchapoton]

comment:13

I modified 3 failing doctests, there just remains one in pickling.

[tscrim]

comment:14

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.

[fchapoton]

comment:15

I am sorry, but I am not quite able to follow your reasoning. If you feel you have a reasonable solution, please proceed.

[tscrim]

comment:16

The problem is

sage: TestParent4() == TestParent4()
True
sage: TestParent4() is TestParent4()

So picking creates a new equal-but-not-identical 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.

[sagetrac-git]

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

ebcd19f

Reverting to previous behavior of allowing comparison of equal parents.

[sagetrac-git]

Changed commit from 7f8d438 to ebcd19f

[tscrim]

comment:18

patchbot is green.

[fchapoton]

comment:19

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 ?

[tscrim]

comment:20

Replying to @fchapoton:

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.

[fchapoton]

comment:21

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 ?

[tscrim]

comment:22

We can if you want, but it will be a low priority for me for the next week.

[vbraun]

Changed branch from public/crystals/remove_cmp_crystals-19488 to ebcd19f