Opened 11 years ago
Closed 7 weeks ago
#8972 closed defect (fixed)
Inversion and fraction fields for power series rings
Reported by:  SimonKing  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  algebra  Keywords:  power series ring, fraction field 
Cc:  tscrim, mkoeppe, nbruin, jhpalmieri, dkrenn, bruno, tmonteil, vdelecroix, ghDaveWitteMorris  Merged in:  
Authors:  Simon King, Michael Jung  Reviewers:  Robert Bradshaw, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  doctest failures 
Branch:  5aca068 (Commits, GitHub, GitLab)  Commit:  5aca06846a0eca565144d9276a13b09282ce34fd 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket is about at least three bugs related with inversion of elements of power series rings.
Here is the first:
sage: R.<x> = ZZ[[]] sage: (1/x).parent() Laurent Series Ring in x over Integer Ring sage: (x/x).parent() Power Series Ring in x over Integer Ring
Both parents are wrong. Usually, the parent of a/b
is the fraction field of the parent of
a,b
, even if
a==b
. And neither above parent is a field.
Next bug:
sage: (1/(2*x)).parent() ERROR: An unexpected error occurred while tokenizing input The following traceback may be corrupted or invalid The error message is: ('EOF in multiline statement', (919, 0))  TypeError Traceback (most recent call last) ... very long traceback TypeError: no conversion of this rational to integer
And the third:
sage: F = FractionField(R) sage: 1/x in F False
With the proposed patch, we solve all bugs except
sage: (x/x).parent() Power Series Ring in x over Integer Ring
Apply (old):
Attachments (7)
Change History (94)
comment:1 Changed 11 years ago by
 Status changed from new to needs_work
 Work issues set to segfault of div of Laurent series
comment:2 Changed 11 years ago by
Strangely, without the patch, the construction works:
sage: R.<x> = ZZ[[]] sage: F = LaurentSeriesRing(FractionField(R.base()),R.variable_names()) sage: 1/F(x) x^1
The patch does this construction as well  but segfaults. There seems to be a nasty side effect.
comment:3 Changed 11 years ago by
The segfault problem seems to come from the fact that the div method for Laurent series relies on the div method for power series  and with my patch, the div method for power series uses the div method for Laurent series. Not good. But that seems solvable.
comment:4 Changed 11 years ago by
 Status changed from needs_work to needs_review
 Work issues segfault of div of Laurent series deleted
OK, I replaced the old patch, and now it seems to work!
For example:
sage: P.<t> = ZZ[] sage: R.<x> = P[[]] sage: 1/(t*x) 1/t*x^1 sage: (1/x).parent() is FractionField(R) True sage: Frac(R) Laurent Series Ring in x over Fraction Field of Univariate Polynomial Ring in t over Integer Ring
The doc tests for the two modified files pass. So, ready for review!
comment:5 Changed 11 years ago by
PS: Even the following works:
sage: 1/(t+x) 1/t  1/t^2*x + (1/t^3)*x^2 + (1/t^4)*x^3 + 1/t^5*x^4  1/t^6*x^5 + 1/t^7*x^6  1/t^8*x^7 + 1/t^9*x^8 + (1/t^10)*x^9 + 1/t^11*x^10 + (1/t^12)*x^11 + 1/t^13*x^12 + (1/t^14)*x^13 + (1/t^15)*x^14 + (1/t^16)*x^15 + (1/t^17)*x^16 + (1/t^18)*x^17 + 1/t^19*x^18  1/t^20*x^19 + O(x^20)
comment:6 followup: ↓ 7 Changed 11 years ago by
I'm wary of such a big change until we have some timing tests in place. In particular, I'm worried about potential slowdowns in the MonskyWashnitzer code.
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Replying to robertwb:
I'm wary of such a big change until we have some timing tests in place. In particular, I'm worried about potential slowdowns in the MonskyWashnitzer code.
You are right, timing should be taken into account, and this is something my patch doesn't provide. Without the patch:
sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 625 loops, best of 3: 1.08 ms per loop
With the patch
sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 125 loops, best of 3: 6 ms per loop
So, it is slower by more than a factor five.
comment:8 followup: ↓ 9 Changed 11 years ago by
 Status changed from needs_review to needs_work
 Work issues set to improve timings
Concerning timings, I see a couple of things that might help improve the div method:
 My patch calls the fraction_field method; in addition the original code calls the laurent_series_ring method. But the purpose of both is the same. So, only one should be done.
 The fraction_field method should better be cached; this would save a lot of time, since the fradtion field construction involves the construction of a Laurent series ring and the construction of the fraction field of the base ring.
 Currently the div methods of power series and of Laurent series call each other; I don't know if this is done efficiently (e.g., avoiding Python calls). I could imagine that this can be stratified.
So, something to do for tomorrow. And I guess the ticket is again "needs work".
comment:9 in reply to: ↑ 8 Changed 11 years ago by
Replying to SimonKing:
Concerning timings, I see a couple of things that might help improve the div method:
One more thing: The old code is quick if the result actually belongs to the power series ring, which is quite often the case; if this is not the case then often an error results. And I guess the parent should always be the fraction field, eventually.
What I just tested (but I really should get some sleep now...) is to cache the fraction_field method, and to try to use the old code if the valuation of the denominator is not bigger than the valuation of the numerator; if this fails, then put numerator and denominator into the fraction field, and try again.
Doing so brings the above timing to about 2ms, which is still a loss of factor two, but two is less than 5 or 6. I'll submit another patch after trying to lift the dependency of the two div methods on each other.
comment:10 Changed 11 years ago by
 Status changed from needs_work to needs_review
 Work issues improve timings deleted
The second patch does several things:
 It turned out that in several places it is assumed that the quotient of two power series is a power series, not a Laurent series. I tried to take care of this.
 I improved the timings, so that (according to the timings below) the performance is competitive (sometimes clearly better, sometimes very little worse) then the original performance.
The reason why the old timings were good was some kind of lazyness: The result of a division was not in the fraction field (as it should be!), but in a simpler ring, so that subsequent computations became easier. On the other hand, transformation into a Laurent polynomial was quite expensive, since there always was a transformation into the Laurent Series Ring's underlying Power Series Ring  even if the given data already belong to it.
Solution (as usual): Add an optional argument "check" to the init method of Laurent Series.
And then I tried to benefit from keeping the results as simple as possible (like the old code did), but in a more proper way. Let f
be a Laurent series in a Laurent series ring
L
. In the old code, one always had
L.power_series_ring() is f.valuation_zero_part().parent()
. I suggest to relax this condition: It is sufficient to have a coercion:
sage: R.<x> = ZZ[[]] sage: f = 1/(1+2*x) sage: f.parent() Laurent Series Ring in x over Rational Field sage: f.parent().power_series_ring() Power Series Ring in x over Rational Field sage: f.power_series().parent() Power Series Ring in x over Integer Ring
Timings
Without the two patches:
sage: R.<x> = ZZ[[]] sage: timeit('a=1/x') 625 loops, best of 3: 291 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 295 µs per loop sage: timeit('a=1/(1+x)') 625 loops, best of 3: 1.07 ms per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 1.07 ms per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 213 µs per loop sage: timeit('y*z') 625 loops, best of 3: 118 µs per loop sage: timeit('yz') 625 loops, best of 3: 212 µs per loop sage: timeit('y/z') ERROR: An unexpected error occurred while tokenizing input The following traceback may be corrupted or invalid The error message is: ('EOF in multiline statement', (27, 0))  ArithmeticError Traceback (most recent call last) ... ArithmeticError: division not defined
With the two patches:
sage: R.<x> = ZZ[[]] sage: timeit('a=1/x') 625 loops, best of 3: 220 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 228 µs per loop sage: timeit('a=1/(1+x)') 625 loops, best of 3: 1.25 ms per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 1.26 ms per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 191 µs per loop sage: timeit('y*z') 625 loops, best of 3: 92.6 µs per loop sage: timeit('yz') 625 loops, best of 3: 191 µs per loop sage: timeit('y/z') 25 loops, best of 3: 9.35 ms per loop
So, my patches not only fix some bugs, but they also slightly improve the performance.
I tested all files that I have changed, but I can not exclude errors in other files.
I forgot to insert my name as an author, but presumably there will be a revision needed anyway, after comments of referees...
comment:11 followup: ↓ 12 Changed 11 years ago by
I forgot one remark:
I don't know how this happens, but the truncate
method of Laurent series behaves different than before, although the
truncate
method of power series did not change:
sage: A.<t> = QQ[[]] sage: f = (1+t)^100 sage: f.truncate(5) 3921225*t^4 + 161700*t^3 + 4950*t^2 + 100*t + 1 sage: f.truncate(5).parent() Univariate Polynomial Ring in t over Rational Field sage: g = 1/f sage: g.truncate(5) 1  100*t + 5050*t^2  171700*t^3 + 4421275*t^4 + O(t^5) sage: g.truncate(5).parent() Laurent Series Ring in t over Rational Field
In other words, g.truncate(5)
is now returning a Laurent series, but without the patch it used to return return a univariate polynomial, similar to
f.truncate(5)
.
I don't know if this is acceptable, and also I don't understand how that happened. Shall I change it?
comment:12 in reply to: ↑ 11 Changed 11 years ago by
Replying to SimonKing:
I forgot one remark:
I don't know how this happens, but the
truncate
method of Laurent series behaves different than before, although the
truncate
method of power series did not change:
Outch, now I see my mistake.
 The documentation of
LaurentSeries?.truncate
states that indeed it returns a Laurent series. It does not have a doc test, so, I will add one.
 The old doc test of
PowerSeries?.truncate
fails with my patch. The reason is that the power series constructed in this doc test is now in fact a Laurent series; this is what made me believe that the behaviour of the method has changed. So, I only have to change the doc test.
But before submitting a patch to add/correct these doc tests, I'll wait for input of a referee.
comment:13 followup: ↓ 14 Changed 11 years ago by
Do you have any timings for power series sqrt()?
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 11 years ago by
Replying to robertwb:
Do you have any timings for power series sqrt()?
Tentatively.
Without the patch:
sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 25 loops, best of 3: 13.3 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 13.6 ms per loop sage: timeit('q = (p^4).sqrt()') 25 loops, best of 3: 14.5 ms per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') ERROR: An unexpected error occurred while tokenizing input The following traceback may be corrupted or invalid The error message is: ('EOF in multiline statement', (27, 0)) ... TypeError: no conversion of this rational to integer sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 25 loops, best of 3: 20.7 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 21.9 ms per loop
With the patch:
sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 25 loops, best of 3: 15.5 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 15.7 ms per loop sage: timeit('q = (p^4).sqrt()') 25 loops, best of 3: 16.6 ms per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') 25 loops, best of 3: 19.2 ms per loop sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 25 loops, best of 3: 23.8 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 24.5 ms per loop
So, over QQ, there is a slight regression (both with exact and inexact input). Over ZZ, it only works with my patch, even for a polynomial that is quadratic by definition.
I will have a closer look at sqrt  perhaps I'll find something.
comment:15 in reply to: ↑ 14 Changed 11 years ago by
Replying to SimonKing:
I will have a closer look at sqrt  perhaps I'll find something.
It seems so.
First, the reason why the old version failed for a power series over ZZ is the fact that the invert method raises an error if the result has coefficients in QQ rather than ZZ. I guess this must change.
Second, there is a division inside the sqrt method. By my patch, this division returns a Laurent series, which slows things slightly down. If I do a*s.invert()
instead of
a/s
then the timings are as good as with the old code.
So, there will soon be a third patch...
Changed 11 years ago by
Improving timings for sqrt, further bug fixes, more doc tests. To be applied after the two other patches
comment:16 Changed 11 years ago by
It was worth it!
First, I found one division bug for Laurent series left, which is now fixed (and doc tested):
sage: L.<x> = LaurentSeriesRing(ZZ) sage: 1/(2+x) 1/2  1/4*x + 1/8*x^2  1/16*x^3 + 1/32*x^4  1/64*x^5 + 1/128*x^6  1/256*x^7 + 1/512*x^8  1/1024*x^9 + 1/2048*x^10  1/4096*x^11 + 1/8192*x^12  1/16384*x^13 + 1/32768*x^14  1/65536*x^15 + 1/131072*x^16  1/262144*x^17 + 1/524288*x^18  1/1048576*x^19 + O(x^20)
This used to result in an error.
Second, I added more doc tests (and inserted my name to the author list).
Third, the new timings for sqrt are fully competitive (even very slightly faster than before), and we still have the bug fix:
Loading Sage library. Current Mercurial branch is: powerseries sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 25 loops, best of 3: 13.2 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 13.5 ms per loop sage: timeit('q = (p^4).sqrt()') 25 loops, best of 3: 14.5 ms per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') 25 loops, best of 3: 16.4 ms per loop sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 25 loops, best of 3: 20.6 ms per loop sage: timeit('q = (p^2).sqrt()') 25 loops, best of 3: 21.7 ms per loop
The trick is that I do s*a.invert()
instead of
s/a
, where
s
and
a
are power series. This helps, because (after a little change) the invert method returns a power series whenever possible  and since I know that
a.valuation()>=0
, I can use fast multiplication of power series rather than slow multiplication of Laurent series.
comment:17 followup: ↓ 18 Changed 11 years ago by
 Status changed from needs_review to needs_work
O dear, I have to mark it "needs work", because the following fails:
sage: R.<x> = ZZ[[]] sage: y = (3*x+2)/(1+x) sage: y=y/x
Changed 11 years ago by
Bugfix for LaurentSeries?._div_; to be applied after the other three patches
comment:18 in reply to: ↑ 17 Changed 11 years ago by
 Status changed from needs_work to needs_review
Replying to SimonKing:
sage: y=y/x
OK, the last (hopefully the last...) patch fixes this. I was verifying the timings for basic arithmetic (the timings without patches are posted above), and here is the positive result:
sage: R.<x> = ZZ[[]] sage: timeit('a=1/x') 625 loops, best of 3: 218 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 228 µs per loop sage: timeit('a=1/(1+x)') 625 loops, best of 3: 1.22 ms per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 1.24 ms per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 189 µs per loop sage: timeit('y*z') 625 loops, best of 3: 92.6 µs per loop sage: timeit('yz') 625 loops, best of 3: 189 µs per loop sage: timeit('y/z') 125 loops, best of 3: 7.1 ms per loop
Now, ready for review, and I'll do something else...
comment:19 Changed 11 years ago by
 Status changed from needs_review to positive_review
Nice work. Apply all 5 patches.
comment:20 Changed 11 years ago by
 Reviewers set to Robert Bradsure
The reviewer field was not filled in. I just added it.
Dave
comment:21 Changed 11 years ago by
 Reviewers changed from Robert Bradsure to Robert Bradshaw
comment:22 Changed 11 years ago by
Sorry about the spelling mistake!
comment:23 Changed 11 years ago by
 Merged in set to sage4.4.4.alpha0
 Resolution set to fixed
 Status changed from positive_review to closed
comment:24 Changed 11 years ago by
 Merged in sage4.4.4.alpha0 deleted
 Status changed from closed to needs_work
I had to back these patches out since they caused lots of errors in schemes/elliptic_curves.
comment:25 Changed 11 years ago by
 Work issues set to trouble with schemes/elliptic_curves
Hi Mike!
That probably means that the elliptic curves code expects the result of a division of two power series to be another power series. But now, the division yields a Laurent series.
I see two possibilities to proceed:
 Search for all occurences of power series in Sage, see if a division occurs, and change it so that things still work.
 Change/extend the methods provided by Laurent series so that they match those provided by power series. In that way, the code in schemes/elliptic_curves still has a chance to work.
I'll see what I can do.
Best regards, Simon
comment:26 followup: ↓ 27 Changed 11 years ago by
I think the problem boils down to two problems:
 Laurent series have no attribute
exp
, unlike power series.
 In some situations, there still occurs a formal fraction field when it should be a Laurent series ring. The result is an attribute error.
For the first situation, I suggest to implement exp
for Laurent series of nonnegative valuation (using exp of the underlying power series). For the second situation, I have to analyse what goes wrong.
comment:27 in reply to: ↑ 26 Changed 11 years ago by
The problem is that some code expects the fraction field of a power series ring to be a formal fraction field. So, assume that one changes it, so that the fraction field is in fact a Laurent polynomial ring. When creating elements of the fraction field, the code fails, because it tries to initialise the element by numerator and denominator  but Laurent series are initialised by a power series and an integer.
A possible solution would be to make the init method of Laurent series accept numerator and denominator. But I think that's a nasty hack, because what happens if the arguments are one power series and one integer? Is the integer supposed to be the valuation of the Laurent series, or the denominator of a formal fraction field element?
comment:28 Changed 11 years ago by
It is really frustrating how many doc tests fail. I solved the problem with exp
and one other attribute error. But there remain many problems:
ZeroDivisionError
, wrong precision of the result (!), and so on.
comment:29 Changed 11 years ago by
In most cases, the problems seem to come from using f/g
on power series in situations where g
is of valuation zero. This used to return a power series, which I consider the wrong answer (it should be a Laurent series). Now, if one wants f/g
as a power series, one should use f * ~g
instead.
I am now confident that I'll soon be able to submit another patch that resolves it.
comment:30 Changed 11 years ago by
 Work issues changed from trouble with schemes/elliptic_curves to fix division if no fraction field exists; use ~ instead of / in elliptic curve code
After fixing what I mentioned above, I get for sage testall
:
The following tests failed: sage t "devel/sage/sage/crypto/lfsr.py" sage t "devel/sage/sage/modular/modform/find_generators.py" sage t "devel/sage/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py"
This seems doable.
Perhaps I was too strict when I implemented _div_
: I wanted that the result always lives in the fraction field, and I wanted that an error is raised if no fraction field exists. But I guess it is better to proceed as it is known from other rings that are no integral domains:
sage: K = ZZ.quo(15) sage: parent(K(3)/K(4)) Ring of integers modulo 15 sage: parent(K(4)/K(3))  ZeroDivisionError Traceback (most recent call last) ... ZeroDivisionError: Inverse does not exist.
So, rule:
 If a fraction field exists then the result of a division must live in it (example: 1/1 is a rational, not an integer).
 If no fraction field exists, then devision shall yield an element of the original ring, if that's possible, and raise a
ZeroDivisionError
otherwise.
Changed 11 years ago by
Fixing some remaining bugs of Laurent/power series arithmetic; fixing doc tests on elliptic curves.
comment:31 Changed 11 years ago by
 Status changed from needs_work to needs_review
 Work issues fix division if no fraction field exists; use ~ instead of / in elliptic curve code deleted
I think the problems are now solved. With the last patch, that is to be applied after all others, sage testall
works without errors (at least in version sage4.4.2).
Changes introduced by the patch
 In some cases, if the code expects a power series, I replaced
a/b
for power seriesa,b
bya*~b
. Namely, the latter returns a power series (not a Laurent series), if possible.
 I added a method
exp
for Laurent series, that returns the exponential if the Laurent series happens to be a power series.
 With my previous patches, the underlying data of a Laurent series is not necessarily a power series. Therefore
add_bigoh
(similarly_rmul_
and_lmul_
) did in some cases not work. This is now fixed and doctested.
 Some Power Series Rings still returned a formal fraction field. This is now fixed, they return a Laurent series ring.
 If
a,b
are elements of a power series ringR
, the rule is now:
 If
R
is an integral domain, thena/b
always belongs toFrac(R)
. This is similar to1/1
being rational, not integer.
 If
R
is no integral domain, thena/b
is an element ofR
or of the Laurent series ring over the base ring ofR
, ifb
is invertible. This is similar to division inZZ.quo(15)
.
 If possible,
~b
is a power series (possibly over the fraction field of the base ofR
). So, we always have~b==1/b
, but the parents may be different. I hope this is acceptable.
 A change in the default
_div_
method ofRingElement
: Previously, the default fora._div_(b)
was to returna.parent().fraction_field()(a,b)
. But this may be a problem (e.g., if the fraction field is not a formal fraction field but a Laurent series ring). Therefore, if the old default results in an error,a.parent().fraction_field(a)/a.parent().fraction_field(b)
is tried.
Timings
Robert was worried about potential slowdowns in the MonskyWashnitzer code. It seems to me that my patches actually provide a considerably speedup.
Without my patches:
sage t "devel/sagepowerseries/sage/schemes/elliptic_curves/monsky_washnitzer.py" [3.9 s]  All tests passed! Total time for all tests: 3.9 seconds
With my patches:
sage t "devel/sagepowerseries/sage/schemes/elliptic_curves/monsky_washnitzer.py" [2.3 s]  All tests passed! Total time for all tests: 2.3 seconds
So, the code seems to become faster by more than 33%!
Ready for review again...
comment:32 Changed 11 years ago by
I upgraded to sage4.4.3.
I verified that all patches still apply.
With the patches, sage testall
passes.
On sagedevel, Robert asked how stable my MonskyWashnitzer timing is. Here is the answer:
Without the patches, I was running sage t "devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py"
5 times, and got
7.9 s, 2.4 s, 2.4 s, 2.4 s, 2.4 s
With the patches, I obtain
2.4 s, 2.5 s, 2.4 s, 2.4 s, 2.4 s
So, the timing did not improve, except for the first run. But at least there was no slowdown.
comment:33 Changed 10 years ago by
Accidentally, I found that this ticket is rotting.
Recall that it used to be "fixed", but then it was reopened by mhansen, since there was a problem in sage4.4.alpha0. Subsequently, I fixed these problems, but still the resolution is set to "fixed".
I guess this is why nobody took care of the ticket afterwards.
I don't know if the patch would still apply (probably not), but at least I verified that the original problem did not disappear.
comment:34 Changed 10 years ago by
 Status changed from needs_review to closed
comment:35 Changed 10 years ago by
 Resolution fixed deleted
 Status changed from closed to new
comment:36 Changed 10 years ago by
 Status changed from new to needs_review
comment:37 Changed 10 years ago by
 Status changed from needs_review to needs_work
test of sage/modular/modform/find_generators.py failed.
./sage t sage/modular/modform/find_generators.py sage t "devel/sagemain/sage/modular/modform/find_generators.py" ********************************************************************** File "/Applications/sage4.6/devel/sagemain/sage/modular/modform/find_generators.py", line 74: sage: span_of_series(v) Exception raised: Traceback (most recent call last): File "/Applications/sage4.6/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/Applications/sage4.6/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/Applications/sage4.6/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_1[8]>", line 1, in <module> span_of_series(v)###line 74: sage: span_of_series(v) File "/Applications/sage4.6/local/lib/python/sitepackages/sage/modular/modform/find_generators.py", line 116, in span_of_series B = [V(g.padded_list(n)) for g in v] File "element.pyx", line 306, in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2666) File "parent.pyx", line 272, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2840) File "parent.pyx", line 170, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2611) AttributeError: 'sage.rings.laurent_series_ring_element.LaurentSeries' object has no attribute 'padded_list' ********************************************************************** File "/Applications/sage4.6/devel/sagemain/sage/modular/modform/find_generators.py", line 79: sage: span_of_series(v,10) Exception raised: Traceback (most recent call last): File "/Applications/sage4.6/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/Applications/sage4.6/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/Applications/sage4.6/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_1[9]>", line 1, in <module> span_of_series(v,Integer(10))###line 79: sage: span_of_series(v,10) File "/Applications/sage4.6/local/lib/python/sitepackages/sage/modular/modform/find_generators.py", line 116, in span_of_series B = [V(g.padded_list(n)) for g in v] File "element.pyx", line 306, in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2666) File "parent.pyx", line 272, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2840) File "parent.pyx", line 170, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2611) AttributeError: 'sage.rings.laurent_series_ring_element.LaurentSeries' object has no attribute 'padded_list' ********************************************************************** 1 items had failures: 2 of 14 in __main__.example_1 ***Test Failed*** 2 failures. For whitespace errors, see the file /Users/sekhon/.sage//tmp/.doctest_find_generators.py [4.0 s]  The following tests failed: sage t "devel/sagemain/sage/modular/modform/find_generators.py" Total time for all tests: 4.0 seconds
comment:38 Changed 10 years ago by
Apply trac8972_fraction_of_power_series_combined.patch
(for the patchbot)
comment:39 Changed 10 years ago by
For simplicity, I produced a patch that combines all previous patches and is rebased to sage4.6.2.alpha4.
I will now try to take care about the failing doc tests.
comment:40 Changed 10 years ago by
 Status changed from needs_work to needs_review
I managed to fix the remaining issues, and all tests pass (unless a last minute change that I just did was a bad idea). Hence, this ticket needs review again.
Since the computation time was a point of critique, let me give you some timings:
Without the patch:
# Basic arithmetic sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 625 loops, best of 3: 803 µs per loop sage: timeit('a=1/x') 625 loops, best of 3: 279 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 285 µs per loop sage: timeit('a=1/(1+x)') 625 loops, best of 3: 801 µs per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 167 µs per loop sage: timeit('y*z') 625 loops, best of 3: 100 µs per loop sage: timeit('yz') 625 loops, best of 3: 166 µs per loop # square roots sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 625 loops, best of 3: 767 µs per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 791 µs per loop sage: timeit('q = (p^4).sqrt()') 625 loops, best of 3: 828 µs per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') <BOOM> sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 625 loops, best of 3: 1 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.04 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.04 ms per loop
With the patch:
# Basic arithmetic sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 625 loops, best of 3: 897 µs per loop sage: timeit('a=1/x') 625 loops, best of 3: 173 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 179 µs per loop sage: timeit('a=1/(1+x)') 625 loops, best of 3: 911 µs per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 900 µs per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 38.4 µs per loop sage: timeit('y*z') 625 loops, best of 3: 23.6 µs per loop sage: timeit('yz') 625 loops, best of 3: 23.6 µs per loop sage: timeit('y/z') 625 loops, best of 3: 309 µs per loop # square roots sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 625 loops, best of 3: 938 µs per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 962 µs per loop sage: timeit('q = (p^4).sqrt()') 625 loops, best of 3: 1 ms per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') 125 loops, best of 3: 2.39 ms per loop sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 625 loops, best of 3: 1.22 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.26 ms per loop
So, in some cases the patch brings a big speedup (such as 38.4 µs versus 167 µs), in other cases it brings a mild slowdown (such as 962 µs versus 791 µs)
Obvious question to the referee: Should I try to squeeze even more µs out of it, or can it stay as it is (assuming that the long tests pass, as I will try now).
comment:41 Changed 10 years ago by
FWIW: Long tests pass.
comment:42 Changed 10 years ago by
Apply trac8972_fraction_of_power_series_combined.patch
comment:43 Changed 10 years ago by
Don't be irritated by the patchbot. The patch is for 4.6.2.alpha4, not for 4.6.1
comment:44 Changed 10 years ago by
 Status changed from needs_review to needs_work
This fails to apply against 4.7, so more rebasing is needed.
cd "/home/kedlaya/Downloads/sage4.7/devel/sage" && hg import "/home/kedlaya/Downloads/trac8972_fraction_of_power_series_combined.patch" applying /home/kedlaya/Downloads/trac8972_fraction_of_power_series_combined.patch patching file sage/modular/overconvergent/genus0.py Hunk #1 FAILED at 1653 1 out of 1 hunks FAILED  saving rejects to file sage/modular/overconvergent/genus0.py.rej patching file sage/rings/power_series_poly.pyx Hunk #4 FAILED at 843 Hunk #5 succeeded at 876 with fuzz 2 (offset 11 lines). 1 out of 5 hunks FAILED  saving rejects to file sage/rings/power_series_poly.pyx.rej patching file sage/rings/power_series_ring_element.pyx Hunk #11 FAILED at 1771 Hunk #12 FAILED at 1783 2 out of 12 hunks FAILED  saving rejects to file sage/rings/power_series_ring_element.pyx.rej abort: patch failed to apply
comment:45 Changed 9 years ago by
 Owner changed from AlexGhitza to (none)
Apparently the trac is locked in "bold face mode". Trying to change it.
comment:46 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I have rebased the patch. The test suite needs to be run, but the patch should now apply to sage5.0.beta10. Here is some supporting evidence:
First section: The bugs to be fixed
sage5.0.beta10gcc without the patch
sage: R.<x> = ZZ[[]] sage: (1/x).parent() # bug Laurent Series Ring in x over Integer Ring sage: (x/x).parent() # bug Power Series Ring in x over Integer Ring sage: (1/(2*x)).parent() # bug Traceback (most recent call last): ... TypeError: no conversion of this rational to integer sage: F = FractionField(R) sage: 1/x in F # bug False sage: F # I think a Laurent series ring would be better Fraction Field of Power Series Ring in x over Integer Ring sage: F(x) x sage: ~F(x) # was a problem with an early form of my patches. 1/x
sage5.0.beta10gcc with the patch
age: R.<x> = ZZ[[]] sage: (1/x).parent() # bug fixed Laurent Series Ring in x over Rational Field sage: (x/x).parent() # bug fixed Laurent Series Ring in x over Rational Field sage: (1/(2*x)).parent() # bug fixed Laurent Series Ring in x over Rational Field sage: F = FractionField(R) sage: 1/x in F # bug fixed True sage: F # I think that's much better! Laurent Series Ring in x over Rational Field sage: F(x) x sage: ~F(x) # new bug avoided x^1
Second section: Timings
Here I am collecting the examples that were studied in the comments above.
sage5.0.beta10gcc without the patch
sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 625 loops, best of 3: 445 µs per loop sage: timeit('a=1/x') 625 loops, best of 3: 283 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 290 µs per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 450 µs per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 104 µs per loop sage: timeit('y*z') 625 loops, best of 3: 76.2 µs per loop sage: timeit('yz') 625 loops, best of 3: 103 µs per loop sage: timeit('y/z') # bug Traceback (most recent call last): ... ArithmeticError: division not defined sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 625 loops, best of 3: 810 µs per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 829 µs per loop sage: timeit('q = (p^4).sqrt()') 625 loops, best of 3: 890 µs per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') # bug ERROR: An unexpected error occurred while tokenizing input The following traceback may be corrupted or invalid The error message is: ('EOF in multiline statement', (2017, 0)) Traceback (most recent call last): ... TypeError: no conversion of this rational to integer sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 125 loops, best of 3: 1.07 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.12 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.12 ms per loop
sage5.0.beta10gcc with the patch
sage: R.<x> = ZZ[[]] sage: timeit('a=1/(1+x)') 625 loops, best of 3: 506 µs per loop sage: timeit('a=1/x') 625 loops, best of 3: 153 µs per loop sage: timeit('a=(1+x)/x') 625 loops, best of 3: 160 µs per loop sage: timeit('a=(1+x)/(1x)') 625 loops, best of 3: 516 µs per loop sage: y = (3*x+2)/(1+x) sage: y=y/x sage: z = (x+x^2+1)/(1+x^4+2*x^3+4*x) sage: z=y/x sage: timeit('y+z') 625 loops, best of 3: 37.8 µs per loop sage: timeit('y*z') 625 loops, best of 3: 26.9 µs per loop sage: timeit('yz') 625 loops, best of 3: 20.6 µs per loop sage: timeit('y/z') # bug fixed 625 loops, best of 3: 277 µs per loop sage: P.<t> = QQ[[]] sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 + O(t^10) sage: p.sqrt() 3  11/2*t  173/24*t^2  5623/144*t^3  174815/1152*t^4  8187925/20736*t^5  12112939/9216*t^6  7942852751/1492992*t^7  1570233970141/71663616*t^8  12900142229635/143327232*t^9 + O(t^10) sage: timeit('q = p.sqrt()') 625 loops, best of 3: 847 µs per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 872 µs per loop sage: timeit('q = (p^4).sqrt()') 625 loops, best of 3: 932 µs per loop sage: PZ = ZZ[['t']] sage: timeit('q = (PZ(p)^2).sqrt()') # bug fixed 125 loops, best of 3: 1.46 ms per loop sage: p = 9  33*t  13*t^2  155*t^3  429*t^4  137*t^5 + 170*t^6 + 81*t^7  132*t^8 + 179*t^9 sage: timeit('q = p.sqrt()') 625 loops, best of 3: 1.11 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.17 ms per loop sage: timeit('q = (p^2).sqrt()') 625 loops, best of 3: 1.17 ms per loop
I think, given that various bugs are fixed, and I think the timings tend to be better (not in all examples, but it seems to me that the loss is less than the gain), I guess it is "needs review", modulo doc test suite.
Apply trac8972_fraction_of_power_series_combined.patch
comment:47 Changed 9 years ago by
 Status changed from needs_review to needs_work
 Work issues set to doctest failure
The patchbot reports a doctest failure with 5.0.beta11:
sage t force_lib devel/sage8972/sage/rings/multi_power_series_ring_element.py ********************************************************************** File "/storage/masiao/sage5.0.beta11patchbot/devel/sage8972/sage/rings/multi_power_series_ring_element.py", line 139: sage: h = 1/f; h Exception raised: Traceback (most recent call last): File "/storage/masiao/sage5.0.beta11patchbot/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/storage/masiao/sage5.0.beta11patchbot/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/storage/masiao/sage5.0.beta11patchbot/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_0[46]>", line 1, in <module> h = Integer(1)/f; h###line 139: sage: h = 1/f; h File "element.pyx", line 1799, in sage.structure.element.RingElement.__div__ (sage/structure/element.c:13260) File "coerce.pyx", line 738, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6583) File "coerce.pyx", line 1210, in sage.structure.coerce.CoercionModel_cache_maps.get_action (sage/structure/coerce.c:11424) File "coerce.pyx", line 1378, in sage.structure.coerce.CoercionModel_cache_maps.discover_action (sage/structure/coerce.c:13135) File "ring.pyx", line 1420, in sage.rings.ring.CommutativeRing._pseudo_fraction_field (sage/rings/ring.c:9929) return self.fraction_field() File "/storage/masiao/sage5.0.beta11patchbot/local/lib/python/sitepackages/sage/rings/power_series_ring.py", line 672, in fraction_field return LaurentSeriesRing(self.base().fraction_field(),self.variable_names()) File "/storage/masiao/sage5.0.beta11patchbot/local/lib/python/sitepackages/sage/rings/laurent_series_ring.py", line 91, in LaurentSeriesRing R = LaurentSeriesRing_field(base_ring, name, sparse) File "/storage/masiao/sage5.0.beta11patchbot/local/lib/python/sitepackages/sage/rings/laurent_series_ring.py", line 400, in __init__ LaurentSeriesRing_generic.__init__(self, base_ring, name, sparse) File "/storage/masiao/sage5.0.beta11patchbot/local/lib/python/sitepackages/sage/rings/laurent_series_ring.py", line 121, in __init__ commutative_ring.CommutativeRing.__init__(self, base_ring, names=name, category=_Fields) File "ring.pyx", line 1365, in sage.rings.ring.CommutativeRing.__init__ (sage/rings/ring.c:9627) Ring.__init__(self, base_ring, names=names, normalize=normalize, File "ring.pyx", line 156, in sage.rings.ring.Ring.__init__ (sage/rings/ring.c:1903) Parent.__init__(self, base=base, names=names, normalize=normalize, File "parent.pyx", line 532, in sage.structure.parent.Parent.__init__ (sage/structure/parent.c:4484) File "parent_gens.pyx", line 331, in sage.structure.parent_gens.ParentWithGens._assign_names (sage/structure/parent_gens.c:3052) File "parent_gens.pyx", line 213, in sage.structure.parent_gens.normalize_names (sage/structure/parent_gens.c:2360) IndexError: the number of names must equal the number of generators **********************************************************************
comment:48 Changed 9 years ago by
Arrgh. That's nasty. The reason for the doctest problems is that meanwhile there are multivariate power series rings  but there are still no multivariate Laurent series rings.
Spontaneously, I suggest a temporary workaround: In the case of multivariate case, the old behaviour (namely: using a formal fraction field) should be used, until multivariate Laurent series are available.
comment:49 Changed 9 years ago by
My spontaneous suggestion does not work easily. Sorry.
comment:50 Changed 8 years ago by
I think it would be great for this ticket to come back to life! Is it possible to just take the current power series classes append _old to all the class names and have the multivariable power series inherit from those?
(Since no one has posted a ticket for creating multivariable Laurent series, let me ask here what is wrong with making them a multivariable power series ring with a univariate Laurent series ring in the background?)
comment:51 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:52 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:53 Changed 7 years ago by
 Branch set to u/pbruin/8972fraction_power_series
 Commit set to 71dfc6b782b4ed291ea0051c4b564bc47c2e510a
Converted the patch into a Git branch and resolved merge conflicts. Fixed a few relatively straightforward doctest failures in the second commit. Summary of remaining failures:
sage t long src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py # 4 doctests failed sage t long src/sage/tests/french_book/polynomes.py # 2 doctests failed sage t long src/sage/rings/multi_power_series_ring_element.py # 4 doctests failed
comment:54 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:55 Changed 7 years ago by
 Branch changed from u/pbruin/8972fraction_power_series to public/ticket/8972
 Commit changed from 71dfc6b782b4ed291ea0051c4b564bc47c2e510a to c29b36db9adad45ded9e6d03b08d90ea61106112
 Work issues changed from doctest failure to doctest failures
just rebased on 6.3.beta5
New commits:
c29b36d  Merge branch 'u/pbruin/8972fraction_power_series' of ssh://trac.sagemath.org:22/sage into 8972

comment:56 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:57 Changed 7 years ago by
 Commit changed from c29b36db9adad45ded9e6d03b08d90ea61106112 to de0ba0d2535f76cd4b0f3a205187c76f0e507cb5
comment:58 Changed 6 years ago by
Failures in src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py seem to be just about slight changes in precision (either gain or loss by one). Maybe one can just correct the result of the doctests.
Failures in src/sage/rings/multi_power_series_ring_element.py are more serious: they come from trying to build a Laurent series ring in several variables, that we currently do not have !
comment:59 Changed 6 years ago by
I have a question about the description of this issue. Please forgive that I'm no algebraist.
sage: (1/(2*x)).parent() ... TypeError: no conversion of this rational to integer
Isn't this correct, with the ring over ZZ? With QQ there is no error.
If so, the description should be corrected.
comment:60 Changed 6 years ago by
 Commit changed from de0ba0d2535f76cd4b0f3a205187c76f0e507cb5 to 888cf7aacc0df223b790e402c23a1506274adf76
Branch pushed to git repo; I updated commit sha1. New commits:
888cf7a  Merge branch 'public/ticket/8972' into 6.5.rc0

comment:61 Changed 6 years ago by
 Commit changed from 888cf7aacc0df223b790e402c23a1506274adf76 to bc2a834ceea7fee072d0275b765693f9792b380e
Branch pushed to git repo; I updated commit sha1. New commits:
bc2a834  trac #8972 correct trivial doctest failures

comment:62 Changed 5 years ago by
 Dependencies set to #21283
Since the power series issue is somewhat separate, I created #21283 to deal with it on its own.
comment:63 Changed 3 months ago by
 Commit changed from bc2a834ceea7fee072d0275b765693f9792b380e to 2602f8d7ad27b954b847514450620f71b37d7289
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2602f8d  Trac #8972: use fraction_field instead of laurent_series

comment:64 followup: ↓ 70 Changed 3 months ago by
 Cc tscrim mkoeppe added
 Status changed from needs_work to needs_review
Approach might be too naive, but it seems to solve the first and the last bug without compromising the timing.
comment:65 Changed 3 months ago by
 Commit changed from 2602f8d7ad27b954b847514450620f71b37d7289 to e4b0977840ebd482fe36c076af76d64197e40213
comment:66 Changed 3 months ago by
comment:67 Changed 3 months ago by
 Commit changed from e4b0977840ebd482fe36c076af76d64197e40213 to 7399856324eaabd8e1bd0b5b12c2e2209454bdbc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7399856  Trac #8972: use fraction_field instead of laurent_series

comment:68 Changed 3 months ago by
This should do.
comment:69 Changed 3 months ago by
 Milestone changed from sage6.4 to sage9.3
comment:70 in reply to: ↑ 64 ; followup: ↓ 73 Changed 3 months ago by
Replying to ghmjungmath:
Approach might be too naive, but it seems to solve the first and the last bug without compromising the timing.
Does this also address the dependency #21283?
comment:71 Changed 3 months ago by
 Cc nbruin added
comment:72 Changed 3 months ago by
 Dependencies #21283 deleted
comment:73 in reply to: ↑ 70 Changed 3 months ago by
Replying to pbruin:
Replying to ghmjungmath:
Approach might be too naive, but it seems to solve the first and the last bug without compromising the timing.
Does this also address the dependency #21283?
Nope. I just wanted to see what the patchbot has to say before I modify the ticket description. Here we go.
comment:74 Changed 3 months ago by
 Description modified (diff)
comment:75 Changed 3 months ago by
Yay! Patchbot is green! :)
comment:76 Changed 3 months ago by
 Cc jhpalmieri added
comment:77 Changed 3 months ago by
 Description modified (diff)
comment:78 Changed 3 months ago by
 Cc dkrenn bruno tmonteil added
Would be nice to have this ticket reviewed. Thanks. :)
comment:79 Changed 3 months ago by
 Cc vdelecroix added
comment:80 Changed 2 months ago by
 Cc ghDaveWitteMorris added
comment:81 Changed 2 months ago by
Patchbot seems still be satisfied with beta7. It'd be nice to have this change in version 9.3 though. Otherwise, a rebase might become necessary soon...
comment:82 followup: ↓ 83 Changed 2 months ago by
There are a few details that still fixing:
This seems strange:
:doc:`laurent_series_ring_element`.
Wouldn't it be better to be a specific reference to the class? Although I am not sure it is worth the direct reference.
This is wrong:
If we try a nonzero, nonunit leading coefficient, we end up in the in the fraction field, i.e. Laurent series ring::
Even for units, you still end up in the fraction field.
However, this must fail if the underlying ring is no integral domain:: +However, this must fail if the underlying ring is not an integral domain::
comment:83 in reply to: ↑ 82 Changed 2 months ago by
Thank you Travis for taking a look!
Replying to tscrim:
There are a few details that still fixing:
This seems strange:
:doc:`laurent_series_ring_element`.Wouldn't it be better to be a specific reference to the class? Although I am not sure it is worth the direct reference.
Alright, I'll change it.
This is wrong:
If we try a nonzero, nonunit leading coefficient, we end up in the in the fraction field, i.e. Laurent series ring::Even for units, you still end up in the fraction field.
Oh right. I think the previous author already meant constant terms instead of leading terms. At least that would make more sense.
However, this must fail if the underlying ring is no integral domain:: +However, this must fail if the underlying ring is not an integral domain::
Will do.
comment:84 Changed 2 months ago by
 Commit changed from 7399856324eaabd8e1bd0b5b12c2e2209454bdbc to 5aca06846a0eca565144d9276a13b09282ce34fd
Branch pushed to git repo; I updated commit sha1. New commits:
5aca068  Trac #8972: documentation improved

comment:85 Changed 2 months ago by
That should be better.
comment:86 Changed 2 months ago by
 Reviewers changed from Robert Bradshaw to Robert Bradshaw, Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:87 Changed 7 weeks ago by
 Branch changed from public/ticket/8972 to 5aca06846a0eca565144d9276a13b09282ce34fd
 Resolution set to fixed
 Status changed from positive_review to closed
OK, here is a patch.
Idea:
With the patch, I get:
O dear. I just realise that there will be more work. There is a segmentation fault, as follows:
So, the division of elements of a Laurent series ring fails with a segmentation fault. By consequence, division of power series segfaults as well, with the patch. "Needs work", I presume.