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: |
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
- Commit changed from ddb72911670163fd031cbdf43062513802df034b to e6bd63b1cbf2959f51e7c7493b99bc9f7b38d3c1
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- 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
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
- 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
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
- 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
- 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
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 8 years ago by
- Commit changed from 868e571e861bea1e96b810ec007d1a676f2d69de to 406dca9676e85adf51ea92b0c8cb65cdef5d2593
comment:11 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:12 Changed 8 years ago by
- Commit changed from 406dca9676e85adf51ea92b0c8cb65cdef5d2593 to e81e7a094ad5ca1fa4a522d9e9c31fde98fa487a
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
- 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
- Commit changed from e81e7a094ad5ca1fa4a522d9e9c31fde98fa487a to 3dd232b6eed83ae6c3a6d177855ee99d2439b919
comment:15 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:16 Changed 7 years ago by
What is the purpose of introducing the (globally available) function lift_centered
?
comment:17 Changed 7 years ago by
- 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
- Commit changed from 3dd232b6eed83ae6c3a6d177855ee99d2439b919 to 584e8740420754ba39325fe281aa1df941edfa25
comment:19 Changed 7 years ago by
- 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
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
- 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
- Branch changed from u/lftabera/lift_centered to u/jdemeyer/lift_centered
comment:23 Changed 7 years ago by
- 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
This is good for me.
comment:25 Changed 7 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to positive_review
comment:26 Changed 7 years ago by
- Branch changed from u/jdemeyer/lift_centered to c2255e586e50d8f07c1098ab2ce05933a1bd5077
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Deprecate centerlift method