#26807 closed defect (fixed)
Cython fixes in Tate algebras
Reported by:  caruso  Owned by:  

Priority:  major  Milestone:  sage8.6 
Component:  padics  Keywords:  
Cc:  roed, jdemeyer  Merged in:  
Authors:  Xavier Caruso  Reviewers:  Travis Scrimshaw, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  c62b226 (Commits)  Commit:  c62b226f4735f3f2f22f92eecb72018544efd27f 
Dependencies:  Stopgaps: 
Description
I address Jeroen's comments in ticket #26195
Change History (16)
comment:1 Changed 9 months ago by
 Branch set to u/caruso/cython_fixes_in_tate_algebras
 Commit set to 50ceccd395602f5f42b3882d413af49624372e18
 Status changed from new to needs_review
comment:2 Changed 9 months ago by
 Commit changed from 50ceccd395602f5f42b3882d413af49624372e18 to 012297514c26facda0bb7a5b11e3cc92fc44d909
comment:3 Changed 9 months ago by
You misinterpreted Jeroen's comment about cpdef _mul_
: it shouldn't be changed to cdef
but instead you need to add cpdef _mul_
to tate_algebra_element.pxd
.
comment:4 Changed 9 months ago by
Why is it better to have cpdef _mul_
than cdef _mul_
?
I noticed that _mul_
is cdef
in MonoidElement
but cpdef
in RingElement
but didn't understand why.
comment:5 Changed 9 months ago by
It's better because it allows Python subclasses to override _mul_
and have it picked up by the coercion system. Moreover, if you use cdef
in a superclass but override it with cpdef
(and possibly also def
), there was a nasty bug in Cython that caused segfaults because Cython tried to call a nonexistent function. That's the source of the warning that Jeroen referred to.
I haven't looked at MonoidElement
, but I would guess that the cdef
_mul_
there should be changed to cpdef
.
comment:6 Changed 9 months ago by
 Commit changed from 012297514c26facda0bb7a5b11e3cc92fc44d909 to 1815392a0a4695833639ee1d6ce5edb26ff50cce
Branch pushed to git repo; I updated commit sha1. New commits:
1815392  Change cdef _mul_ to cpdef _mul_

comment:7 Changed 9 months ago by
 Status changed from needs_review to needs_work
This should be cpdef _floordiv_
:
cdef _floordiv_(self, other):
Also, fill in your name as Author.
comment:8 Changed 9 months ago by
In _cmp_c
, shouldn't we be worried about potential overflow here?
cdef int c = other._valuation_c()  self._valuation_c()
The result of _valuation_c
is a long
and you are downcasting that to int
.
The easiest solution is returning a long
in _cmp_c
.
comment:9 Changed 9 months ago by
 Commit changed from 1815392a0a4695833639ee1d6ce5edb26ff50cce to 5cb4670656c98b89b50caf74c767f66f30048101
Branch pushed to git repo; I updated commit sha1. New commits:
5cb4670  cpdef _floordiv_

comment:11 Changed 9 months ago by
 Commit changed from 5cb4670656c98b89b50caf74c767f66f30048101 to c62b226f4735f3f2f22f92eecb72018544efd27f
Branch pushed to git repo; I updated commit sha1. New commits:
c62b226  long _cmp_c

comment:12 Changed 9 months ago by
Jeroen, David, any other comments here? If not, I will flip this to positive.
comment:13 Changed 9 months ago by
 Reviewers set to Travis Scrimshaw, Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:14 Changed 9 months ago by
Thanks; I'm happy with the changes.
comment:15 Changed 8 months ago by
 Branch changed from u/caruso/cython_fixes_in_tate_algebras to c62b226f4735f3f2f22f92eecb72018544efd27f
 Resolution set to fixed
 Status changed from positive_review to closed
comment:16 Changed 8 months ago by
 Milestone changed from sage8.5 to sage8.6
This tickets were closed as fixed after the Sage 8.5 release.
Branch pushed to git repo; I updated commit sha1. New commits:
Remove import rich_to_bool (not used)