Opened 4 years ago
Closed 3 years ago
#25219 closed enhancement (fixed)
Implement reverse() for LaurentSeries
Reported by: | gh-BrentBaccala | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.8 |
Component: | algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Brent Baccala | Reviewers: | Vincent Delecroix |
Report Upstream: | N/A | Work issues: | |
Branch: | ebd8f45 (Commits, GitHub, GitLab) | Commit: | ebd8f4500c2cdc09d701ffab9105fe72e9f4506d |
Dependencies: | Stopgaps: |
Description (last modified by )
Power series have a reverse()
method for series of valuation 1.
This enhancement wraps the power series method and makes it available for LaurentSeries of valuation 1.
sage: R.<x> = Frac(QQ[['x']]) sage: f = 2*x + 3*x^2 - x^4 + O(x^5) sage: g = f.reverse() sage: g 1/2*x - 3/8*x^2 + 9/16*x^3 - 131/128*x^4 + O(x^5) sage: f(g) x + O(x^5) sage: g(f) x + O(x^5)
Change History (17)
comment:1 Changed 4 years ago by
- Branch set to u/gh-BrentBaccala/25219
- Commit set to d7ed82816e9274eebb570cc1f1697b35a39d81f3
comment:2 Changed 4 years ago by
- Status changed from new to needs_review
comment:3 Changed 4 years ago by
- Commit changed from d7ed82816e9274eebb570cc1f1697b35a39d81f3 to 90ab7ed44057b4a02b70f9b169632155fca22bd9
Branch pushed to git repo; I updated commit sha1. New commits:
90ab7ed | Merge tag '8.4' into u/gh-BrentBaccala/25219
|
comment:4 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.5
comment:5 Changed 3 years ago by
Rather than type(self)(self._parent, rev)
couldn't you use the parent call method? (something like self._parent(rev)
?
comment:6 Changed 3 years ago by
- Milestone changed from sage-8.5 to sage-8.7
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to needs_info
Also, I am not able to reproduce the bug that you mention in the ticket description on 8.7.beta6...
comment:7 Changed 3 years ago by
- Milestone changed from sage-8.7 to sage-8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:8 Changed 3 years ago by
- Commit changed from 90ab7ed44057b4a02b70f9b169632155fca22bd9 to c2e486c3be77a50f064beda43bf11c5df6d4e97d
comment:9 Changed 3 years ago by
- Commit changed from c2e486c3be77a50f064beda43bf11c5df6d4e97d to e86ed66b7be556602c8173fafbdcc5cd45aba73e
Branch pushed to git repo; I updated commit sha1. New commits:
e86ed66 | Trac #25219: add test case
|
comment:10 Changed 3 years ago by
- Description modified (diff)
- Status changed from needs_info to needs_review
Code updated as suggested.
I can't reproduce the bug anymore, either. I added it as a test case.
comment:11 follow-up: ↓ 12 Changed 3 years ago by
Does power series really behave this way
+ if rev.parent() == u.parent(): + return self._parent(rev) + else: + P = self._parent.change_ring(rev.parent().base_ring()) + return P(rev)
It is very counterintuitive to have methods whose output type depends on the nature of the answer. If you do 4/2
in Sage you do not obtain an integer but a rational number. I don't understand why .reverse()
should be any different.
I would suggest to have reverse
and reverse_unit
.
I am just asking for your opinion as this is beyond the scope of the ticket.
comment:12 in reply to: ↑ 11 Changed 3 years ago by
Replying to vdelecroix:
Does power series really behave this way
+ if rev.parent() == u.parent(): + return self._parent(rev) + else: + P = self._parent.change_ring(rev.parent().base_ring()) + return P(rev)It is very counterintuitive to have methods whose output type depends on the nature of the answer. If you do
4/2
in Sage you do not obtain an integer but a rational number. I don't understand why.reverse()
should be any different.I would suggest to have
reverse
andreverse_unit
.I am just asking for your opinion as this is beyond the scope of the ticket.
I chose to mimic the behavior of the existing reverse
method in PowerSeries_poly
. Its docstring states:
If the leading coefficient is not a unit, we pass to its fraction field if possible
The Laurent series code calls the power series code, and follows its lead. So the answer to your question is yes, power series really do behave this way:
sage: R.<x> = ZZ[[]] sage: (2*x+x^2).parent() Power Series Ring in x over Integer Ring sage: (2*x+x^2).reverse() 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) sage: (2*x+x^2).reverse().parent() Power Series Ring in x over Rational Field
I would tend to think that it should work this way, that 4/2
should return an integer and only 3/2
should return a rational, but I haven't thought about it enough to have a really informed opinion.
comment:13 follow-up: ↓ 15 Changed 3 years ago by
- Status changed from needs_review to needs_info
The following test is working for me
sage: B.<b> = PolynomialRing(ZZ) sage: A.<t> = LaurentSeriesRing(B) sage: f = 2*b*t + b*t^2 + 3*b^2*t^3 + O(t^4) sage: g = f.reverse() sage: g(f) # known bug - f fails to coerce properly, but next test works t + O(t^4)
Why is it discarded?
comment:14 Changed 3 years ago by
- Commit changed from e86ed66b7be556602c8173fafbdcc5cd45aba73e to ebd8f4500c2cdc09d701ffab9105fe72e9f4506d
Branch pushed to git repo; I updated commit sha1. New commits:
ebd8f45 | Trac #25219: remove TEST that was already an EXAMPLE
|
comment:15 in reply to: ↑ 13 Changed 3 years ago by
Replying to vdelecroix:
The following test is working for me
sage: B.<b> = PolynomialRing(ZZ) sage: A.<t> = LaurentSeriesRing(B) sage: f = 2*b*t + b*t^2 + 3*b^2*t^3 + O(t^4) sage: g = f.reverse() sage: g(f) # known bug - f fails to coerce properly, but next test works t + O(t^4)Why is it discarded?
It used to fail. I don't know what changed to fix it, but it was somewhere else in Sage. I'll bisect the code if you really want to track it down.
Once it was working, I added it as a test case, but forgot that it was already there as a "known bug" example. Fixed.
comment:16 Changed 3 years ago by
- Status changed from needs_info to positive_review
comment:17 Changed 3 years ago by
- Branch changed from u/gh-BrentBaccala/25219 to ebd8f4500c2cdc09d701ffab9105fe72e9f4506d
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac #25219: add reverse() method to LaurentSeries