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:

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 mmasdeu 9 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by roed

  • Status changed from new to needs_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 9 years ago by roed

  • Status changed from needs_review to needs_work

comment:3 Changed 9 years ago by mmasdeu

  • Status changed from needs_work to needs_review

OK. Done.

comment:4 Changed 9 years ago by roed

  • 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 mmasdeu

  • 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 mmasdeu

comment:6 Changed 9 years ago by roed

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 roed

  • Reviewers set to David Roe

comment:8 Changed 9 years ago by robertwb

  • 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 klee

  • 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 roed

Fine with me.

comment:11 Changed 9 years ago by jdemeyer

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

comment:12 Changed 9 years ago by klee

  • Authors set to Marc Masdeu

comment:13 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.3.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.