Skip to content
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

Add documentation to LaurentSeries point to accessors #39366

Merged
merged 8 commits into from
Feb 21, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Jan 22, 2025

I feel it's originally too undiscoverable. Since f and n are explicitly mentioned in the documentation I think it's a good idea to mention how to access it programmatically.

📝 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 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 7002dbd; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 4, 2025

I don't know. The original docstring looks simple and to the point.

How about adding a short introduction to the head of the doc

https://doc-pr-39366--sagemath.netlify.app/html/en/reference/power_series/sage/rings/laurent_series_ring_element

where you can explain about f and n?

@user202729 user202729 marked this pull request as draft February 4, 2025 04:40
@kwankyu
Copy link
Collaborator

kwankyu commented Feb 6, 2025

Thanks.

I noticed that, in the doc, f is doubly used to represent a Laurent series and also to represent the power series part of its internal representation. To solve this, perhaps we may use a different name for the power series, like $f = t^n g$ where $g$ is a power series in $t$ of valuation zero.

Since your added explanation is fundamental, I suggest putting it to the very beginning of the doc, before the first example. You may also add an example to support it.

If you think the more work is out of the scope of your PR, just let me know. This is still an improvement. I may work upon it later.

@user202729
Copy link
Contributor Author

user202729 commented Feb 10, 2025

I think it's not that fundamental, but more like if someone need to implement some function that manipulates Laurent series and need to deconstruct it, it is the most efficient to use the accessors instead of, say, .list() or something.

Thus the original explanation being only in the "IMPLEMENTATION:" section. Of course if the implementation decides to be changed for some reason, .valuation() and .valuation_zero_part() would still stay there, but potentially not as efficient as a single method access.

But maybe like this? It's also possible to rename the valuation_zero_part to u to be consistent with the internal method name __u.

diff --git a/src/sage/rings/laurent_series_ring_element.pyx b/src/sage/rings/laurent_series_ring_element.pyx
index 758b7012240..211dab70de8 100644
--- a/src/sage/rings/laurent_series_ring_element.pyx
+++ b/src/sage/rings/laurent_series_ring_element.pyx
@@ -1,6 +1,16 @@
 r"""
 Laurent Series
 
+Laurent series in Sage are represented internally
+as a power of the variable times the unit part (which need not be a
+unit - it's a polynomial with nonzero constant term). The zero
+Laurent series has unit part 0.
+
+For a Laurent series `f` internally represented as `f = t^n \cdot g` where
+`t` is the variable and `g` has nonzero constant term, `g` can be accessed
+through :meth:`valuation_zero_part` and `n` can be accessed through
+:meth:`valuation`.
+
 EXAMPLES::
 
     sage: R.<t> = LaurentSeriesRing(GF(7), 't'); R
@@ -35,15 +45,6 @@ Saving and loading.
     sage: loads(K.dumps()) == K                                                         # needs sage.rings.real_mpfr
     True
 
-IMPLEMENTATION: Laurent series in Sage are represented internally
-as a power of the variable times the unit part (which need not be a
-unit - it's a polynomial with nonzero constant term). The zero
-Laurent series has unit part 0.
-
-For a Laurent series internally represented as `t^n \cdot f` where
-`t` is the variable, `f` can be accessed through :meth:`valuation_zero_part`
-and `n` can be accessed through :meth:`valuation`.
-
 AUTHORS:
 
 - William Stein: original version
@@ -93,8 +94,8 @@ cdef class LaurentSeries(AlgebraElement):
     r"""
     A Laurent Series.
 
-    We consider a Laurent series of the form `t^n \cdot f` where `f` is a
-    power series.
+    We consider a Laurent series of the form `f = t^n \cdot g` where `g` is a
+    power series with nonzero constant term.
 
     INPUT:
 

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 10, 2025

Just one paragraph (without details uninteresting to users):

Laurent series in Sage are represented internally as a power of the variable
times the power series part. If a Laurent series `f` is represented as 
`f = t^n \cdot g` where `t` is the variable and `g` has nonzero constant term, 
`g` can be accessed through :meth:`valuation_zero_part` and `n` can be accessed 
through :meth:`valuation`.

?

or with u instead of g.

@user202729 user202729 marked this pull request as ready for review February 12, 2025 07:26
@user202729 user202729 marked this pull request as draft February 12, 2025 07:28
@user202729 user202729 marked this pull request as ready for review February 12, 2025 09:51
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39366: Add documentation to LaurentSeries point to accessors
    
I feel it's originally too undiscoverable. Since `f` and `n` are
explicitly mentioned in the documentation I think it's a good idea to
mention how to access it programmatically.

### 📝 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#39366
Reported by: user202729
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit b57e337 into sagemath:develop Feb 21, 2025
19 of 24 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