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:

Status badges

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)

trac_12612_padic_is_unit_fix.patch (4.7 KB) - added by Marc Masdeu 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by David Roe

Status: newneeds_review

I think you need to add an explanation in the docstring of is_unit explaining why the change has been made, and pointing at the is_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.

comment:2 Changed 11 years ago by David Roe

Status: needs_reviewneeds_work

comment:3 Changed 11 years ago by Marc Masdeu

Status: needs_workneeds_review

OK. Done.

comment:4 Changed 11 years ago by David Roe

Status: needs_reviewneeds_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 Marc Masdeu

Status: needs_workneeds_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 Marc Masdeu

comment:6 Changed 11 years ago by David Roe

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 David Roe

Reviewers: David Roe

comment:8 Changed 10 years ago by Robert Bradshaw

Reviewers: David RoeDavid 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 Kwankyu Lee

Reviewers: David Roe, Robert BradshawDavid Roe, Robert Bradshaw, Kwankyu Lee
Status: needs_reviewpositive_review

Looks nice. Why wait?

comment:10 Changed 10 years ago by David Roe

Fine with me.

comment:11 Changed 10 years ago by Jeroen Demeyer

mmasdeu: please fill in the author field with your real name.

comment:12 Changed 10 years ago by Kwankyu Lee

Authors: Marc Masdeu

comment:13 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.3.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.