Opened 8 years ago

Closed 7 years ago

#15804 closed enhancement (fixed)

Add lift_centered for more classes

Reported by: lftabera Owned by:
Priority: major Milestone: sage-7.0
Component: basic arithmetic Keywords: modular arithmetic, centered lift
Cc: Merged in:
Authors: Luis Felipe Tabera Alonso, Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: c2255e5 (Commits, GitHub, GitLab) Commit: c2255e586e50d8f07c1098ab2ce05933a1bd5077
Dependencies: Stopgaps:

Status badges

Description

This ticket is a split of #8558 for easier review and merging.

Add a lift_centered method to ntl_mod_p elements and a generic function.

If p is an integer mod n, returns r congruent to p mod n such that -n/2 < r <= n/2

It also deprecates the use of centerlift for lift_centered. The discussion is in #8558

Change History (26)

comment:1 Changed 8 years ago by git

  • Commit changed from ddb72911670163fd031cbdf43062513802df034b to e6bd63b1cbf2959f51e7c7493b99bc9f7b38d3c1

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

e6bd63bDeprecate centerlift method

comment:2 Changed 8 years ago by lftabera

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I would write the global function lift_centered as

try:
    return p.lift_centered()
except AttributeError:
    return p.lift()

such that it always works on modular objects, even if .lift_centered() has no meaning.

comment:4 Changed 8 years ago by lftabera

Are you thinking for instance?

K.<x,y,z> = QQ[]
I = Ideal(x*y,x*z,y*z)
R = K.quotient_ring(I)
f = R((x+y+z)**2)
lift_centered(f)
x^2 + y^2 + z^2

So what you propose is that lift_centered should be always a class method and never explicitely computed using the global function, don't you?

comment:5 Changed 8 years ago by git

  • Commit changed from e6bd63b1cbf2959f51e7c7493b99bc9f7b38d3c1 to 169cb08369fc0dda69badcbeae5c41e13304a54d

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

169cb08Make global lift_centered function fallback to lift if the object

comment:6 Changed 8 years ago by jdemeyer

In Python, it is better not to use hasattr. Instead, catch AttributeError.

Also, the normal syntax is except AttributeError: (without parentheses).

comment:7 Changed 8 years ago by git

  • Commit changed from 169cb08369fc0dda69badcbeae5c41e13304a54d to 868e571e861bea1e96b810ec007d1a676f2d69de

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

868e571Catch exceptions instead of using hassattr.

comment:8 Changed 8 years ago by lftabera

  • Status changed from needs_work to needs_review

Fixed, I have also changed the documentation, I hope it is in better shape now.

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 8 years ago by git

  • Commit changed from 868e571e861bea1e96b810ec007d1a676f2d69de to 406dca9676e85adf51ea92b0c8cb65cdef5d2593

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

9da4eabMerge branch 'master' into u/lftabera/lift_centered
406dca9Fix import error

comment:11 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:12 Changed 8 years ago by git

  • Commit changed from 406dca9676e85adf51ea92b0c8cb65cdef5d2593 to e81e7a094ad5ca1fa4a522d9e9c31fde98fa487a

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

e81e7a0Merge branch 'master' into u/lftabera/lift_centered

comment:13 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

For the newly added docstrings, you should follow the format explained in http://www.sagemath.org/doc/developer/coding_basics.html#documentation-strings. Note that the INPUT and OUTPUT blocks should not be indented and that the docstring shouldn't start with an empty line.

comment:14 Changed 7 years ago by git

  • Commit changed from e81e7a094ad5ca1fa4a522d9e9c31fde98fa487a to 3dd232b6eed83ae6c3a6d177855ee99d2439b919

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

2f30718Merge tag '6.5' into u/lftabera/lift_centered
fff5565Merge tag '6.6' into u/lftabera/lift_centered
7a5e891Merge tag '6.7' into u/lftabera/lift_centered
3dd232bTicket 15804

comment:15 Changed 7 years ago by lftabera

  • Status changed from needs_work to needs_review

comment:16 Changed 7 years ago by vdelecroix

What is the purpose of introducing the (globally available) function lift_centered?

comment:17 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-6.10
  • Status changed from needs_review to needs_work

13 has not been addressed.

Also, for PARI, you should not deprecate centerlift() since that's the PARI name of the method. You may allow both lift_centered() and centerlift() but do not deprecate centerlift().

comment:18 Changed 7 years ago by git

  • Commit changed from 3dd232b6eed83ae6c3a6d177855ee99d2439b919 to 584e8740420754ba39325fe281aa1df941edfa25

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

a53a956Merge tag '6.10.beta6' into u/lftabera/lift_centered
584e874Trac #15804: Add lift_centered for more classes

comment:19 Changed 7 years ago by lftabera

  • Status changed from needs_work to needs_review

Ups, sorry, the docstring format was not good. I have fixed it.

@vdelecroix: After thinking about it, the global namespace is already too big. It should be used for essential functions only. And preferably those that are "classical" or have sense for an arbitrary Sage element. So, I have deleted the global function, the user should depend on the method of each class.

comment:20 follow-up: Changed 7 years ago by vdelecroix

Couldn't you do some economy in pari.gen

cdef class gen(gen_auto):
   ...
   lift_centered = centerlift

comment:21 in reply to: ↑ 20 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-6.10 to sage-7.0
  • Status changed from needs_review to needs_work

Replying to vdelecroix:

Couldn't you do some economy in pari.gen

cdef class gen(gen_auto):
   ...
   lift_centered = centerlift

I agree.

comment:22 Changed 7 years ago by jdemeyer

  • Branch changed from u/lftabera/lift_centered to u/jdemeyer/lift_centered

comment:23 Changed 7 years ago by jdemeyer

  • Authors changed from Luis Felipe Tabera Alonso to Luis Felipe Tabera Alonso, Jeroen Demeyer
  • Commit changed from 584e8740420754ba39325fe281aa1df941edfa25 to c2255e586e50d8f07c1098ab2ce05933a1bd5077
  • Status changed from needs_work to needs_review

New commits:

c2255e5For PARI, define lift_centered as alias of centerlift

comment:24 Changed 7 years ago by vdelecroix

This is good for me.

comment:25 Changed 7 years ago by jdemeyer

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:26 Changed 7 years ago by vbraun

  • Branch changed from u/jdemeyer/lift_centered to c2255e586e50d8f07c1098ab2ce05933a1bd5077
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.