8000 Allow coercion from Frac(QQ[x]) to LaurentSeriesRing(QQ) by user202729 · Pull Request #39365 · sagemath/sage · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow coercion from Frac(QQ[x]) to LaurentSeriesRing(QQ) #39365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

user202729
Copy link
Contributor
@user202729 user202729 commented Jan 22, 2025

If A is a field then self = LaurentSeriesRing(A) is a field, by universal property of fraction field, a coercion P.base() → self can be uniquely extended to P → self. So this makes sense.

As a side effect, this coercion allows the user to write things such as

sage: R.<x> = QQ[]
sage: 1/(x-1)+O(x^100)
-1 - x - x^2 - x^3 - x^4 - x^5 - x^6 - x^7 - x^8 - x^9 - x^10 - x^11 - x^12 - x^13 - x^14 - x^15 - x^16 - x^17 - x^18 - x^19 + O(x^20)

Another side effect: If the base ring is something like Zmod(5) or QQ["x"].quotient(...) then a primality/irreducibility check will be carried out.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have 8000 created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link
github-actions bot commented Jan 28, 2025

Documentation preview for this PR (built with commit 9a90328; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@fchapoton
Copy link
Contributor

Maybe an option would to replace 'A.is_field()byA in Fields()`. I am not sure, but this may avoid triggering the primality check.

@user202729
Copy link
Contributor Author

A in Fields() triggers the primality check anyway if I recalled correctly.

What doesn't is A.category().is_subcategory(Fields()). But do we really want to do that?

(on the positive side once A has its category refined to Fields() then it will work but… does caching cause any issue?)

@fchapoton
Copy link
Contributor

You are probably right. So we have the choice either to do nothing and avoid triggering the primality check, or do what you propose. I think that we can accept the cost of primality testing. So I agree to give a positive review.

By the way, thanks a lot for your many pull requests, that I find very good and that are helping us to improve sage. I am currently at a Sage Days meeting, and several of us are appreciating your contributions.

@user202729
Copy link
Contributor Author

Thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39365: Allow coercion from Frac(QQ[x]) to LaurentSeriesRing(QQ)
    
If `A` is a field then `self = LaurentSeriesRing(A)` is a field, by
universal property of fraction field, a coercion `P.base() → self` can
be uniquely extended to `P → self`. So this makes sense.

As a side effect, this coercion allows the user to write things such as

```sage
sage: R.<x> = QQ[]
sage: 1/(x-1)+O(x^100)
-1 - x - x^2 - x^3 - x^4 - x^5 - x^6 - x^7 - x^8 - x^9 - x^10 - x^11 -
x^12 - x^13 - x^14 - x^15 - x^16 - x^17 - x^18 - x^19 + O(x^20)
```

Another side effect: If the base ring is something like `Zmod(5)` or
`QQ["x"].quotient(...)` then a primality/irreducibility check will be
carried out.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39365
Reported by: user202729
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39485:  Implement conversion from laurent series to rational function field
    
As in the title. I think this behavior makes sense because `QQ(RR(…))`
currently uses `.simplest_rational()` instead of like exact value (like
`x / 2^53` for some integer `x`), so it makes sense for similar
conversions to compute a good approximation too.

I cannot prove that this round-trips, however.

Note that currently conversion from power series `QQ[[x]]` to polynomial
ring `QQ[x]` truncates, because of implementation detail this leads to
conversion from `QQ[[x]]` to `Frac(QQ[x])` also truncates. I think this
is undesirable behavior because identical-looking elements in `QQ[[x]]`
versus `Frac(QQ[[x]]) = LaurentSeriesRing(QQ, "x")` has different
conversion behavior.

Elements which are already Laurent polynomial are preserved.

Side note: `.one()` is faster because it's cached

```sage
sage: R.<x> = QQ[]
sage: S = Frac(R)
sage: %timeit S.one()  # fast because it's cached
90.6 ns ± 1.27 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops
each)
sage: ring_one = S.ring().one()
sage: %timeit S._element_class(S, ring_one, ring_one, coerce=False,
reduce=False)
3.42 μs ± 75 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops
each)
```

Maybe this doesn't need to be explicitly written in the documentation
(users trying to do the conversion will just see the result)

There exists also <code>:meth:\`.PowerSeries_poly.pade\`</code> which
should probably be mentioned in the documentation.

Reverse direction (that one is a morphism):
sagemath#39365

Possible caveat:

```
sage: S(Frac(V)(1/(x+1))/(x^10))
1/(x^11 + x^10)
sage: S(Frac(V)(1/(x+1))/(x^30))
(-x^19 + x^18 - x^17 + x^16 - x^15 + x^14 - x^13 + x^12 - x^11 + x^10 -
x^9 + x^8 - x^7 + x^6 - x^5 + x^4 - x^3 + x^2 - x + 1)/x^30
```

It could be argued the latter would be simpler as `1/(x^31+x^30)`, but
then that way gives higher denominator degree.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39485
Reported by: user202729
Reviewer(s): Travis Scrimshaw, user202729
@vbraun vbraun merged commit 357f95e into sagemath:develop Feb 21, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0