py3 remove __cmp__ from Element

[chapoton]

as another step to py3

[chapoton]

let us wait for the bots

---

New commits:

​5ad0b9b

py3: get rid of a spurious cmp() in element.pyx

[jdemeyer]

This is obviously not correct: you cannot just remove that branch.

[chapoton]

could you please explain why ? no doctest fail.

[chapoton]

bot is green.

[jdemeyer]

Replying to chapoton:

could you please explain why ? no doctest fail.

That's not how it works. You have to explain me why the change that you made makes sense. And "no doctest fail" is never a good explanation.

I'm not even saying that the change is wrong, but it certainly needs to be justified.

[jdemeyer]

Replying to jdemeyer:

I'm not even saying that the change is wrong

Well, ok, I did say that, but that might have been too harsh.

[jdemeyer]

If you really want to forward to Python 3, why not remove __cmp__ completely?

[git]

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

​0145889

Remove generic __cmp__ from Element

[jdemeyer]

This doesn't really work because there are Element classes which have different semantics for __cmp__ and __richcmp__. So it's not easy to do this.

[chapoton]

so, can I put back my branch and set it to needs review ?

it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)

[jdemeyer]

Replying to chapoton:

so, can I put back my branch and set it to needs review ? it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)

I looked at your branch and it is wrong because it happens after coercion. There is no guarantee that the result of coercion will be Sage elements.

[jdemeyer]

Anyway, I think it's a bad idea to try to remove those 3 lines. You should look at __cmp__ as a whole and fix that. It makes little sense to do a micro-change like what you suggest.

[chapoton]

Replying to jdemeyer:

Replying to chapoton:

so, can I put back my branch and set it to needs review ? it seems to me that what I remove can never happen (because this is a method of the element class, self is an Element)

I looked at your branch and it is wrong because it happens after coercion. There is no guarantee that the result of coercion will be Sage elements.

oh ! really ? I thought that when things have a parent, they are elements. Would you have an example where this is not the case ?

Other possibilities that come to my mind : what would you think of replacing "cmp(left, right)" by "left.__cmp__(right)" (I have not yet tried if it works) ? or maybe by "raise TypeError('coercion should build members of Element class')" ?

This is one of the latest calls to cmp() in a cython file. I would really like to be able to cythonize all the pxy files with python3.

[chapoton]

Replying to jdemeyer:

This doesn't really work because there are Element classes which have different semantics for __cmp__ and __richcmp__. So it's not easy to do this.

Do you remember how many failures there are, and where ? how many classes are concerned by this bad double semantic ? I would suspect that this happens at least in symbolic ring and real numbers. This may also be the cause of the problems in #22257.

[jdemeyer]

Replying to chapoton:

oh ! really ? I thought that when things have a parent, they are elements.

Of course. That's trivially true, since a parent is a concept which was invented for the Element/Parent classes.

Would you have an example where this is not the case ?

Not right away, but I know that it's possible.

Other possibilities that come to my mind : what would you think of replacing "cmp(left, right)" by "left.__cmp__(right)"

I don't really like that, but this might be the least bad thing to do.

"raise TypeError('coercion should build members of Element class')" ?

Certainly not this.

[jdemeyer]

Replying to chapoton:

oh ! really ? I thought that when things have a parent, they are elements. Would you have an example where this is not the case ?

OK, I have an example: the coercion of RealField(54)(1) and float(1.0) gives a float.

This might be considered a bug though...

[git]

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

​a2310fc

Replace parent_c -> parent

[jdemeyer]

Oops, wrong ticket...

[jdemeyer]

Replying to chapoton:

Replying to jdemeyer:

This doesn't really work because there are Element classes which have different semantics for __cmp__ and __richcmp__. So it's not easy to do this.

Do you remember how many failures there are, and where ?

I didn't really keep track. Certainly intervals too.

[chapoton]

putting Jeroen's branch back, so that one can run a bot on it

---

New commits:

​a2310fc

Replace parent_c -> parent

[chapoton]

trying with another branch, that just remove blankly __cmp__

this is a test branch, so that one can run a patchbot on it

---

New commits:

​8ff1e79

test branch for removal of __cmp__

[chapoton]

some preparation work done in #22446, #22447, #22448

[chapoton]

some of the problems (including the ones in Coxeter groups and heegner.py) reduces to the following one:

sage: A1=QuadraticField(5)
sage: A2=QuadraticField(5)
sage: A1==A2
False

or the existing doctest

QuadraticField(-11, 'a') is QuadraticField(-11, 'a')

[chapoton]

so we probably really should fix #22257 first

[chapoton]

now waiting at least for #22890, #22815 and #22907

[git]

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

​fe632c4

Merge branch 'public/22297' in 8.0.b7

[git]

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

​d05cd3f

just remove __cmp__ completely in element.pyx

[git]

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

​7a7afb8

trying to remove __cmp__ from element.pyx

[chapoton]

another preparation step is #23133

[git]

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

​b616111

Merge branch 'public/22297' in 8.0.b10

[chapoton]

maybe the last preparation step is #23273

[chapoton]

this should be ready, let us wait for a patchbot report

[chapoton]

ok, bot is essentially green. Please review.

[jdemeyer]

Great news!

One detail: you should keep the doctest (changing cmp and __cmp__ to rich comparisons accordingly). Perhaps move that doctest to _richcmp_.

[jdemeyer]

Also, only this

            left_cmp = left.__cmp__

should be inside the try statement.

[git]

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

​43f21e1	Merge branch 'public/22297' in 8.0.b12
​87ade9d	trac 22297 keeping the former doctest inside _richcmp_

[chapoton]

ok for the doctest. I also did something for the try statement, but was not sure of what you meant.

[jdemeyer]

I meant something like

        try:
            left_cmp = left.__cmp__
        except AttributeError:
            pass
        else:
            if isinstance(left_cmp, MethodType):
                return left_cmp(right)

but I'm wondering if we still the check if isinstance(left_cmp, MethodType):. Maybe this would work too:

        try:
            left_cmp = left.__cmp__
        except AttributeError:
            pass
        else:
            return left_cmp(right)

[git]

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

​0b39903

trac 22297 without isinstance check

[chapoton]

oh, I see. I did not know this syntax.

Let us try without the isinstance check then.

[jdemeyer]

You don't need left_cmp = None, just write pass in the except clause.

[git]

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

​6124122

trac 22297 detail

[chapoton]

done, sorry

[chapoton]

bot is morally green

EDIT now squarely green