Inherit __richcmp__ and __cmp__ in subclasses of Element

[jdemeyer]

Use the __typeinit__ mechanism introduced by #18330 to inherit __cmp__ and __richcmp__ even if those would not be inherited by default.

Upstream patch added: https://github.com/cython/cython/pull/383

Upstream: Fixed upstream, but not in a stable release.

CC: @jpflori

Component: coercion

Author: Jeroen Demeyer

Branch: f6058e7

Reviewer: Jean-Pierre Flori

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

ac96e64

Merge branch 'develop' into t/18329/ticket/18329

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4b08519 to ac96e64

[jpflori]

comment:33

Did you submit the Cython patch upstream?

Another question, is there any doc about this in the dev manual or is there only the comments in element.pyx? I guess the latter comments should at least be modified.

Finally it seems to me that at the moment, this change only affects complex_mpc.pyx and real_double.pyx. Is that correct?

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 Use the `__typeinit__` mechanism introduced by #18330 to inherit `__cmp__` and `__richcmp__` even if those would not be inherited by default.
+
+**Upstream patch added**: [https://github.com/cython/cython/pull/383](https://github.com/cython/cython/pull/383)

[jdemeyer]

comment:34

Replying to @jpflori:

Did you submit the Cython patch upstream?

Yes.

Finally it seems to me that at the moment, this change only affects complex_mpc.pyx and real_double.pyx. Is that correct?

Sure. I changed those as "proof of concept" to show that my patch works.

[jdemeyer]

Upstream: Fixed upstream, but not in a stable release.

[jpflori]

comment:35

Ok, that is good news, I was kind of afraid that this patch was not so useful :)

Do you plan on opening a follow-up ticket to remove all the now superfluous boilerplate code?

I guess my only concern left is about the comments in elements.pyx.

[jdemeyer]

comment:36

Replying to @jpflori:

Another question, is there any doc about this in the dev manual

As far as I know, there is not. There is some documentation in src/doc/en/thematic_tutorials/coercion_and_categories.rst, but that's really about Python only.

I do plan to write documentation in #18306, but I haven't gotten around to actually do it.

or is there only the comments in element.pyx? I guess the latter comments should at least be modified.

I'll have a look.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

0e0301a

Fix documentation

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from ac96e64 to 0e0301a

[jpflori]

Reviewer: Jean-Pierre Flori

[jpflori]

comment:38

Thanks!

[jdemeyer]

comment:39

Replying to @jpflori:

Do you plan on opening a follow-up ticket to remove all the now superfluous boilerplate code?

Since this would be ticket with a lot of potential conflicts, it's best if the author and potential reviewer agree in advance that such a thing be a good idea and to try to get that ticket merged quickly after a new beta comes out.

Can you do that?

[vbraun]

comment:40

sage -t --long src/sage/structure/dynamic_class.py
**********************************************************************
File "src/sage/structure/dynamic_class.py", line 358, in sage.structure.dynamic_class.dynamic_class_internal
Failed example:
    inspect.getfile(Foo2)
Expected:
    '.../sage/structure/dynamic_class.pyc'
Got:
    '/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/structure/dynamic_class.py'
**********************************************************************
File "src/sage/structure/dynamic_class.py", line 360, in sage.structure.dynamic_class.dynamic_class_internal
Failed example:
    inspect.getfile(Foo3)
Expected:
    '.../sage/structure/dynamic_class.pyc'
Got:
    '/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/structure/dynamic_class.py'
**********************************************************************
1 item had failures:
   2 of  20 in sage.structure.dynamic_class.dynamic_class_internal
    [69 tests, 2 failures, 0.49 s]

[jdemeyer]

comment:41

Any clue why the extension changed from .pyc to .py? Can we just change the doctest? This doesn't really look related to this ticket.

[vbraun]

comment:42

I think its because import dynamic_class was removed somewhere, changing the internal caching. IMHO the doctest should be using sage.misc.sageinspect.sage_getfile()

[jdemeyer]

comment:43

I cannot reproduce this problem... suggestions?

[jdemeyer]

comment:44

Are you really sure that this ticket is causing that problem?

[jdemeyer]

comment:45

Cannot reproduce on home.vbraun.cc either...

[vbraun]

comment:46

Can we still replace it with sage.misc.sageinspect.sage_getfile which afaik normalized pyc -> py filenames? I don't have time to try to reproduces this and its just waiting for trouble anyways. Might be due to timing issues if it depends on import order, too.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

f6058e7

Use sage_getfile instead of inspect.getfile

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 0e0301a to f6058e7

[jdemeyer]

New commits:

f6058e7

Use sage_getfile instead of inspect.getfile

[vbraun]

Changed branch from u/jdemeyer/ticket/18329 to f6058e7

[jdemeyer]

Changed commit from f6058e7 to none

[jdemeyer]

comment:50

Replying to @jpflori:

Do you plan on opening a follow-up ticket to remove all the now superfluous boilerplate code?

One such ticket (with not so many changes) is #18881.