Opened 11 years ago
Closed 10 years ago
#12612 closed defect (fixed)
Fix is_unit() in padics so that it is mathematically correct
Reported by: | Marc Masdeu | Owned by: | David Roe |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.3 |
Component: | padics | Keywords: | padics is_unit |
Cc: | David Roe | Merged in: | sage-5.3.beta0 |
Authors: | Marc Masdeu | Reviewers: | David Roe, Robert Bradshaw, Kwankyu Lee |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Consider the following code:
sage: R = Qp(5,5) sage: S.<x> = PowerSeriesRing(R,3) sage: f=5+x sage: f**(-1) 5^-1 + O(5^4) + (4*5^-2 + 4*5^-1 + 4 + 4*5 + 4*5^2 + O(5^3))*x + (5^-3 + O(5^2))*x^2 + O(x^3) sage: f.is_unit() False
This does not make mathematical sense, and the reason for this behavior is that is_unit() in p
-adics does not return whether a given element can be inverted in its parent ring. Instead, it returns whether the element has valuation zero. Since the function is_unit() is used in generic algorithms (for instance when trying to invert a power series) it should return what its name promises.
I attach a patch with corrected output and doctests.
Attachments (1)
Change History (14)
comment:1 Changed 11 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
comment:4 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
There's a failing test in sage/rings/padics/tests.py
coming from the implementation of log()
in sage/rings/padics/padic_base_generic_element.pyx
that calls is_unit
. You should either change that test to is_padic_unit()
or make this ticket depend on 12575.
comment:5 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
I changed the log to use is_padic_unit() and it passes the tests in tests.py. I guess this would have disappeared anyway when the logs and exps get the new implementation...which hopefully will be reviewed soon (hint, hint).
Changed 11 years ago by
Attachment: | trac_12612_padic_is_unit_fix.patch added |
---|
comment:6 Changed 11 years ago by
I've been looking at it. I'll probably be able to review it on Friday: I'm giving a talk tomorrow that I need to prepare for.
I'll also review this on Friday, but I don't anticipate it needing any more changes.
comment:7 Changed 11 years ago by
Reviewers: | → David Roe |
---|
comment:8 Changed 10 years ago by
Reviewers: | David Roe → David Roe, Robert Bradshaw |
---|
Wow, nice catch. I'm surprised there's not more code using this function (though I guess a lot of the internal p-adic code is manipulating things at a lower level).
I hope we don't break other users--I'm tempted to put in a warning, but this should be fixed. Thoughts? Otherwise, positive review.
comment:9 Changed 10 years ago by
Reviewers: | David Roe, Robert Bradshaw → David Roe, Robert Bradshaw, Kwankyu Lee |
---|---|
Status: | needs_review → positive_review |
Looks nice. Why wait?
comment:12 Changed 10 years ago by
Authors: | → Marc Masdeu |
---|
comment:13 Changed 10 years ago by
Merged in: | → sage-5.3.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
I think you need to add an explanation in the docstring of
is_unit
explaining why the change has been made, and pointing at theis_padic_unit
function as a replacement for the old behavior. Other than that, the patch looks fine in principle, though I need to run the doctests.