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: sage-7.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 tscrim

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

comment:2 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/19488
  • Commit set to 1113e99f2d62cffa4c6dfc5064e52322ad416567

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:

1113e99trac 19488 converting cmp to richcmp in affine crystal
Last edited 4 years ago by chapoton (previous) (diff)

comment:3 Changed 4 years ago by tscrim

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 4 years ago by git

  • Commit changed from 1113e99f2d62cffa4c6dfc5064e52322ad416567 to 40ede9e3bebe42618af46ce06163b65f89d5f052

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

40ede9etrac 19488 using richcmp via coercion

comment:5 Changed 4 years ago by chapoton

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 4 years ago by chapoton

  • Milestone changed from sage-6.10 to sage-7.4

comment:7 Changed 4 years ago by tscrim

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.

Last edited 4 years ago by tscrim (previous) (diff)

comment:8 Changed 4 years ago by chapoton

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 ?

Last edited 4 years ago by chapoton (previous) (diff)

comment:9 Changed 4 years ago by chapoton

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 4 years ago by git

  • Commit changed from 40ede9e3bebe42618af46ce06163b65f89d5f052 to 18307b3fbdb825fee35e3cb0e492dfeac157bfa1

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

caa8475Merge branch 'u/chapoton/19488' in 7.4.b2
18307b3trac 19488 more changes in cmp for subcrystals

comment:11 Changed 4 years ago by tscrim

  • Authors set to Frédéric Chapoton, Travis Scrimshaw
  • Branch changed from u/chapoton/19488 to public/crystals/remove_cmp_crystals-19488
  • 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:

6f36d1aChanging ElementWrapper.__richcmp__ to _richcmp_.

comment:12 Changed 4 years ago by git

  • Commit changed from 6f36d1a0fc47848ffc083faae69f9f46df75713e to 7f8d438404a118b99c92238e4da5bedf494e0f81

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

7f8d438trac 19488 fixing some of the failing doctests

comment:13 Changed 4 years ago by chapoton

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

comment:14 Changed 4 years ago by tscrim

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 chapoton

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 tscrim

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.

comment:17 Changed 4 years ago by git

  • Commit changed from 7f8d438404a118b99c92238e4da5bedf494e0f81 to ebcd19f0d1d8f8f2f89ed22666143a7f3ea5de15

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

ebcd19fReverting to previous behavior of allowing comparison of equal parents.

comment:18 Changed 4 years ago by tscrim

patchbot is green.

comment:19 follow-up: Changed 4 years ago by chapoton

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 tscrim

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 chapoton

  • 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 tscrim

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 vbraun

  • Branch changed from public/crystals/remove_cmp_crystals-19488 to ebcd19f0d1d8f8f2f89ed22666143a7f3ea5de15
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.