Opened 9 years ago
Closed 9 years ago
#12612 closed defect (fixed)
Fix is_unit() in padics so that it is mathematically correct
Reported by: | mmasdeu | Owned by: | roed |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.3 |
Component: | padics | Keywords: | padics is_unit |
Cc: | roed | 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 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 9 years ago by
- Status changed from needs_review to 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 9 years ago by
- Status changed from needs_work to 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 9 years ago by
comment:6 Changed 9 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 9 years ago by
- Reviewers set to David Roe
comment:8 Changed 9 years ago by
- Reviewers changed from David Roe to 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 9 years ago by
- Reviewers changed from David Roe, Robert Bradshaw to David Roe, Robert Bradshaw, Kwankyu Lee
- Status changed from needs_review to positive_review
Looks nice. Why wait?
comment:10 Changed 9 years ago by
Fine with me.
comment:11 Changed 9 years ago by
mmasdeu: please fill in the author field with your real name.
comment:12 Changed 9 years ago by
comment:13 Changed 9 years ago by
- Merged in set to sage-5.3.beta0
- Resolution set to fixed
- Status changed from positive_review to 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.