Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#26807 closed defect (fixed)

Cython fixes in Tate algebras

Reported by: caruso Owned by:
Priority: major Milestone: sage-8.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 4 months ago by caruso

  • Branch set to u/caruso/cython_fixes_in_tate_algebras
  • Commit set to 50ceccd395602f5f42b3882d413af49624372e18
  • Status changed from new to needs_review

comment:2 Changed 4 months ago by git

  • Commit changed from 50ceccd395602f5f42b3882d413af49624372e18 to 012297514c26facda0bb7a5b11e3cc92fc44d909

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

0122975Remove import rich_to_bool (not used)

comment:3 Changed 4 months ago by roed

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 4 months ago by caruso

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 4 months ago by roed

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 non-existent 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 4 months ago by git

  • Commit changed from 012297514c26facda0bb7a5b11e3cc92fc44d909 to 1815392a0a4695833639ee1d6ce5edb26ff50cce

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

1815392Change cdef _mul_ to cpdef _mul_

comment:7 Changed 4 months ago by jdemeyer

  • Status changed from needs_review to needs_work

This should be cpdef _floordiv_ (which should then also be declared the .pxd file):

cdef _floordiv_(self, other):

Also, fill in your name as Author.

Last edited 4 months ago by jdemeyer (previous) (diff)

comment:8 Changed 4 months ago by jdemeyer

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

  • Commit changed from 1815392a0a4695833639ee1d6ce5edb26ff50cce to 5cb4670656c98b89b50caf74c767f66f30048101

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

5cb4670cpdef _floordiv_

comment:10 Changed 4 months ago by caruso

  • Authors set to Xavier Caruso
  • Status changed from needs_work to needs_review

Done.

comment:11 Changed 4 months ago by git

  • Commit changed from 5cb4670656c98b89b50caf74c767f66f30048101 to c62b226f4735f3f2f22f92eecb72018544efd27f

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

c62b226long _cmp_c

comment:12 Changed 3 months ago by tscrim

Jeroen, David, any other comments here? If not, I will flip this to positive.

comment:13 Changed 3 months ago by jdemeyer

  • Reviewers set to Travis Scrimshaw, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:14 Changed 3 months ago by roed

Thanks; I'm happy with the changes.

comment:15 Changed 3 months ago by vbraun

  • 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 3 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.6

This tickets were closed as fixed after the Sage 8.5 release.

Note: See TracTickets for help on using tickets.