Opened 3 years ago

Closed 2 years ago

#22297 closed enhancement (fixed)

py3 remove __cmp__ from Element

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch, jhpalmieri Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 6124122 (Commits) Commit: 61241224f0b95c91e6000108669de70cb2e827c6
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

as another step to py3

Change History (49)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/22297
  • Commit set to 5ad0b9b5468636871ccb73366263a5caac008dcf
  • Status changed from new to needs_review

let us wait for the bots


New commits:

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

comment:2 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:3 follow-up: Changed 3 years ago by chapoton

could you please explain why ? no doctest fail.

comment:4 Changed 3 years ago by chapoton

  • Cc tscrim jdemeyer aapitzsch jhpalmieri added

bot is green.

comment:5 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by jdemeyer

  • Authors changed from Frédéric Chapoton to Jeroen Demeyer
  • Description modified (diff)
  • Summary changed from py3 remove cmp() in element.pyx to py3 remove __cmp__ from Element

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

comment:8 Changed 3 years ago by jdemeyer

  • Branch changed from u/chapoton/22297 to u/jdemeyer/22297

comment:9 Changed 3 years ago by git

  • Commit changed from 5ad0b9b5468636871ccb73366263a5caac008dcf to 01458896eb6cb1d79573b58c0f5fc74a0e644d15

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

0145889Remove generic __cmp__ from Element

comment:10 follow-up: Changed 3 years ago by 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.

comment:11 follow-up: Changed 3 years ago by 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)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by 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.

comment:13 Changed 3 years ago by 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.

comment:14 in reply to: ↑ 12 ; follow-ups: Changed 3 years ago by 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.

comment:15 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by 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.

comment:16 in reply to: ↑ 14 Changed 3 years ago by 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.

comment:17 in reply to: ↑ 14 Changed 3 years ago by 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...

comment:18 Changed 3 years ago by git

  • Commit changed from 01458896eb6cb1d79573b58c0f5fc74a0e644d15 to a2310fc7da8d2c4e59789f9840eaabe294cf59dd

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

a2310fcReplace parent_c -> parent

comment:19 Changed 3 years ago by jdemeyer

  • Branch u/jdemeyer/22297 deleted
  • Commit a2310fc7da8d2c4e59789f9840eaabe294cf59dd deleted

Oops, wrong ticket...

comment:20 Changed 3 years ago by jdemeyer

  • Authors Jeroen Demeyer deleted

comment:21 in reply to: ↑ 15 Changed 3 years ago by 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.

comment:22 Changed 3 years ago by chapoton

  • Branch set to u/jdemeyer/22297
  • Commit set to a2310fc7da8d2c4e59789f9840eaabe294cf59dd

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


New commits:

a2310fcReplace parent_c -> parent

comment:23 Changed 3 years ago by chapoton

  • Branch changed from u/jdemeyer/22297 to public/22297
  • Commit changed from a2310fc7da8d2c4e59789f9840eaabe294cf59dd to 8ff1e7973267aa08986f0e645612f0bec17c74c3

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:

8ff1e79test branch for removal of __cmp__

comment:24 Changed 3 years ago by chapoton

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

comment:25 Changed 3 years ago by 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')

In turn, this seems to come from problems in RealLazyField?.

from sage.rings.number_field.number_field import NumberFieldFactory
nff = NumberFieldFactory("number_field_factory")
sage: R.<x> = QQ[]
a1=nff.create_object(None, (QQ, x^2 -2, ('a',), RLF(sqrt(2)), None, None, False, None), check=False)
a2=nff.create_object(None, (QQ, x^2 -2, ('a',), RLF(sqrt(2)), None, None, False, None), check=False)
a1 == a2

where the failure of equality comes from the RLF terms.

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

comment:26 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.6 to sage-8.0

so we probably really should fix #22257 first

comment:27 Changed 3 years ago by chapoton

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

comment:28 Changed 3 years ago by git

  • Commit changed from 8ff1e7973267aa08986f0e645612f0bec17c74c3 to fe632c44f09d5df73438a66f9f501a2a0a0ca46e

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

fe632c4Merge branch 'public/22297' in 8.0.b7

comment:29 Changed 3 years ago by git

  • Commit changed from fe632c44f09d5df73438a66f9f501a2a0a0ca46e to d05cd3f214728effcbc0a7af73186b2401d14c83

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

d05cd3fjust remove __cmp__ completely in element.pyx

comment:30 Changed 3 years ago by git

  • Commit changed from d05cd3f214728effcbc0a7af73186b2401d14c83 to 7a7afb83f71076133e83d960d8234c8f566680ca

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

7a7afb8trying to remove __cmp__ from element.pyx

comment:31 Changed 3 years ago by chapoton

another preparation step is #23133

comment:32 Changed 3 years ago by git

  • Commit changed from 7a7afb83f71076133e83d960d8234c8f566680ca to b6161114435c936a65bd7ce5b45b81627dff6fac

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

b616111Merge branch 'public/22297' in 8.0.b10

comment:33 Changed 2 years ago by chapoton

maybe the last preparation step is #23273

comment:34 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

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

comment:35 Changed 2 years ago by chapoton

ok, bot is essentially green. Please review.

comment:36 Changed 2 years ago by jdemeyer

  • Authors set to Frédéric Chapoton
  • Status changed from needs_review to needs_work

Great news!

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

comment:37 Changed 2 years ago by jdemeyer

Also, only this

            left_cmp = left.__cmp__

should be inside the try statement.

comment:38 Changed 2 years ago by git

  • Commit changed from b6161114435c936a65bd7ce5b45b81627dff6fac to 87ade9d5f8b3bac02cf13fca33d418b8ca56457d

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

43f21e1Merge branch 'public/22297' in 8.0.b12
87ade9dtrac 22297 keeping the former doctest inside _richcmp_

comment:39 Changed 2 years ago by chapoton

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

comment:40 Changed 2 years ago by 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)

comment:41 Changed 2 years ago by git

  • Commit changed from 87ade9d5f8b3bac02cf13fca33d418b8ca56457d to 0b3990339945037981e0975fcd61a9a28c1c3c8f

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

0b39903trac 22297 without isinstance check

comment:42 Changed 2 years ago by chapoton

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

Let us try without the isinstance check then.

comment:43 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:44 Changed 2 years ago by jdemeyer

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

comment:45 Changed 2 years ago by git

  • Commit changed from 0b3990339945037981e0975fcd61a9a28c1c3c8f to 61241224f0b95c91e6000108669de70cb2e827c6

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

6124122trac 22297 detail

comment:46 Changed 2 years ago by chapoton

done, sorry

comment:47 Changed 2 years ago by chapoton

bot is morally green

EDIT now squarely green

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

comment:48 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:49 Changed 2 years ago by vbraun

  • Branch changed from public/22297 to 61241224f0b95c91e6000108669de70cb2e827c6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.