Opened 8 years ago

Closed 7 years ago

# Add lift_centered for more classes

Reported by: Owned by: lftabera major sage-7.0 basic arithmetic modular arithmetic, centered lift Luis Felipe Tabera Alonso, Jeroen Demeyer Vincent Delecroix N/A c2255e5 c2255e586e50d8f07c1098ab2ce05933a1bd5077

### 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

### 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:

 ​e6bd63b `Deprecate 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:

 ​169cb08 `Make 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:

 ​868e571 `Catch 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:

 ​9da4eab `Merge branch 'master' into u/lftabera/lift_centered` ​406dca9 `Fix 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

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

 ​e81e7a0 `Merge 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:

 ​2f30718 `Merge tag '6.5' into u/lftabera/lift_centered` ​fff5565 `Merge tag '6.6' into u/lftabera/lift_centered` ​7a5e891 `Merge tag '6.7' into u/lftabera/lift_centered` ​3dd232b `Ticket 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

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:

 ​a53a956 `Merge tag '6.10.beta6' into u/lftabera/lift_centered` ​584e874 `Trac #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: ↓ 21 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

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:

 ​c2255e5 `For 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.