Opened 11 years ago
Closed 10 years ago
#4477 closed enhancement (fixed)
[with patch; positive review] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term
Reported by: | sengupta | Owned by: | sengupta |
---|---|---|---|
Priority: | minor | Milestone: | sage-3.2.2 |
Component: | basic arithmetic | Keywords: | |
Cc: | dmharvey | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The patch posted against this ticket enhances the exp() function for PowerSeriesRing? elements and allows it to compute with valid non-zero constant terms in the power series.
Previously: f.exp() would return an error "Constant term must be zero" if f[0] != 0
With the patch: f.exp() checks if f[0].exp() is at all defined and in the case it is defined, whether f[0].exp() belongs to the base ring of f. If both the conditions are satisfied, f.exp() is returned.
Attachments (2)
Change History (15)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Cc dmharvey added
- Owner changed from somebody to sengupta
- Status changed from new to assigned
comment:2 Changed 11 years ago by
I will review this in the next few days.
comment:3 Changed 11 years ago by
- Milestone changed from sage-3.2 to sage-3.2.1
comment:4 follow-up: ↓ 6 Changed 11 years ago by
Code is basically good. Still running doctests. A few comments:
- Indenting on "Check for non-zero constant term" line is wrong. Please fix.
- Shouldn't have "fixes \#4477" (what will that mean in two years)
- please refactor so that self.derivative().solve_linear_de(prec) is not called in two different places
- "if C.parent()!=self.base_ring()" should use "is not" instead of "!=" (much more efficient)
- possibly should use "if not self[0].is_zero()" instead of "if self[0]". I can't remember why, but maybe is_zero actually does something useful (I vaguely remember it might have something to do with testing zero-ness in inexact rings)
- the error message "exponential of the input does not belong to the ring" should be more useful to the user. Perhaps "exponential of the constant term does not lie in the coefficient ring. Consider coercing to an appropriate larger ring", or if you're feeling ambitious, invoke the coercion machinery to automagically find that larger ring (but don't ask me how to do that, I have no idea)
- I think that error message is also misleading in the case that the constant term doesn't have an exp method defined. Perhaps raise a different error in that case, e.g. "cannot exponentiate series; exp of constant term is not defined".
- The last doctest is misleading. The power series ring is defined over QQ, but the doctest *implicitly* creates a power series over RR and then exponentiates that. To demonstrate the different situation clearly, the doctest should explicitly create the power series ring over RR.
comment:5 Changed 11 years ago by
- Summary changed from [with patch; needs review] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term to [with patch; needs work] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term
comment:6 in reply to: ↑ 4 Changed 11 years ago by
Replying to dmharvey:
- Shouldn't have "fixes \#4477" (what will that mean in two years)
I disagree. Any doctest marked that way should never be changed since it fixes a specific bug and hence refers to the trac ticket. Tests like that should probably be moved to a TESTS section in the long term, but we can deal with that later.
Cheers,
Michael
comment:7 Changed 11 years ago by
comment:8 Changed 11 years ago by
ok, fair enough. At some point it starts getting kind of cluttered though, and can distract from the "documentation" purpose of the docstring. I agree with moving it out to a TESTS section at some point.
Changed 10 years ago by
comment:9 Changed 10 years ago by
- Summary changed from [with patch; needs work] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term to [with patch; needs review] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term
Second attempt, to address my previous concerns. Apply both patches.
comment:10 Changed 10 years ago by
Is this expected?
sage: R.<x> = ZZ[[]] sage: x.exp() Traceback (most recent call last): File "<ipython console>", line 1, in <module> File "power_series_ring_element.pyx", line 1383, in sage.rings.power_series_ring_element.PowerSeries.exp (sage/rings/power_series_ring_element.c:9850) File "power_series_ring_element.pyx", line 1305, in sage.rings.power_series_ring_element.PowerSeries.solve_linear_de (sage/rings/power_series_ring_element.c:9707) File "power_series_ring_element.pyx", line 1648, in sage.rings.power_series_ring_element._solve_linear_de (sage/rings/power_series_ring_element.c:11103) File "power_series_ring_element.pyx", line 1648, in sage.rings.power_series_ring_element._solve_linear_de (sage/rings/power_series_ring_element.c:11103) File "power_series_ring_element.pyx", line 1650, in sage.rings.power_series_ring_element._solve_linear_de (sage/rings/power_series_ring_element.c:11124) File "/Users/robert/sage/sage-3.1.3/local/lib/python2.5/site-packages/sage/rings/polynomial/polynomial_ring.py", line 252, in __call__ return C(self, x, check, is_gen, construct=construct) File "polynomial_integer_dense_flint.pyx", line 224, in sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint.__init__ (sage/rings/polynomial/polynomial_integer_dense_flint.cpp:4981) File "parent.pyx", line 293, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:3828) File "parent.pyx", line 284, in sage.structure.parent.__call__ (sage/structure/parent.c:3712) File "rational.pyx", line 2288, in sage.rings.rational.Q_to_Z._call_ (sage/rings/rational.c:14682) TypeError: no conversion of this rational to integer
The sqrt function automatically passes to the rationals.
sage: (1+x).sqrt() 1 + 1/2*x - 1/8*x^2 + 1/16*x^3 - 5/128*x^4 + 7/256*x^5 - 21/1024*x^6 + 33/2048*x^7 - 429/32768*x^8 + 715/65536*x^9 - 2431/262144*x^10 + 4199/524288*x^11 - 29393/4194304*x^12 + 52003/8388608*x^13 - 185725/33554432*x^14 + 334305/67108864*x^15 - 9694845/2147483648*x^16 + 17678835/4294967296*x^17 - 64822395/17179869184*x^18 + 119409675/34359738368*x^19 + O(x^20)
comment:11 Changed 10 years ago by
I suppose it's reasonable to expect it automatically passes to the quotient field where possible, but unless someone wants to fix that right now, it should probably go onto another ticket.
(While we're here, shouldn't the sqrt function pass to the localisation away from the prime 2? Why all the way to Q? (Just kidding --- I know the answer :-)))
comment:12 Changed 10 years ago by
- Summary changed from [with patch; needs review] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term to [with patch; positive review] Allow exp() function for PowerSeriesRing element to compute with valid non-zero constant term
I've created #4748. Other than that it works great (and the issue above didn't work before either).
- Robert
comment:13 Changed 10 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged trac_4477.patch and trac_4477-2.patch in Sage 3.2.2.alpha1
Hi David ... I have just posted the patch to the exp() code ... Please take a look ... It will be great if I can have it checked ... Thank you ...