-
-
Notifications
You must be signed in to change notification settings - Fork 636
Added enhancement for power series that allows access to the coefficients of specific terms #39480
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
Added enhancement for power series that allows access to the coefficients of specific terms #39480
Conversation
…ients of specific terms
Thanks for the enhancement. I will set to positive review on Wednesday if there are no other comments and However, I think the initial docstring can be clarified. Perhaps something like:
|
Documentation preview for this PR (built with commit d6e3515; changes) is ready! 🎉 |
Sorry, I thought I already told you this, but it's not in my comment above, so apparently not: you need to change |
I'm a bit split on this solution, because it makes power series behave differently from everything else. Did you think about the possibility of a more generic solution? It would be good to at least try to unify the interface for polynomials, power series and lazy power series. |
I think this is a clear improvement over the status quo. Not only do I agree with the statement on the original issue that "the absence of this core functionality for multivariate series is a clear deficiency...", but this change also makes multivariable power series behave more like polynomials, which is a step toward unification of the interfaces. I think the unification should be achieved by adding this functionality to the other classes, not by handicapping this one. On the other hand, I don't see a good reason to have the single-integer interface. Should this be deprecated, and replaced with a Would it be easy to add the functionality of this method to lazy power series? I don't know anything about them. |
Co-authored-by: Martin Rubey <[email protected]>
Added small fixes suggested by mantepse. |
I opened issue #39698 for adding the same functionality to |
This is not true. For univariate power series and univariate polynomial,
I suspect this was not too well-thought-out and just added to access the components of the object (because the is internally implemented as follows
) For what it's worth singular multivariate polynomial ring already support this. But they also support single integer (return the coefficient, not the polynomial).
They also have That said, I think it's a bad idea to have different behavior for
this PR doesn't conflict with that goal, so it's fine. |
Sorry, I had no time, but I am quite concerned about the general strategy. Is this urgent to go in? |
Not sure what you mean — do you agree with my proposal of general strategy described above? If yes then this can be merged just fine. Or do you think having |
Sorry, I miscommunicated and additionally made a mistake by not distinguishing between homogeneous component and coefficient in the univariate case. I was focussed on the problem that, at least for I am very unsure whether it is a good idea to make, say,
It doesn't seem completely inconsistent to me to return a coefficient in the univariate case - in my mind I identify sequences with formal power series. But I am very unsure here, too. @tscrim, what do you think? I somewhat doubt that it is realistic to have gradings other than the $\mathbb N` grading here. If the parent is a
|
For consistency of API, it's either that or adding a specialized method like
Actually some types of weighted grading is already implemented. (to some extent, see #37155 )
Okay, this is just a technical issue and can be fixed by checking |
I think it is fully consistent to return a coefficient in the univariate case and a polynomial in the multivariate case. It is about decomposing the vector space into its graded components. So in this sense, the API is uniform. (Note: Polynomials are not considered as graded objects for morphism reasons, but otherwise we treat them as For the power series rings, the grading on the polynomial rings is important because we use that to define the valuation, which then gives the completion. In particular, here we only consider the total degree. For power series rings, we could make both accessible by differentiating a tuple versus integer input. However, this will not be possible when working with a completion in general as the both could be indexed by |
On the other hand there is more structure in the ring of polynomials than a vector space, for example you can multiply two coefficients together. Anyway, what do you think |
Okay, an algebra into its graded components, but that is only an isomorphism of vector spaces anyways. The most consistent thing to me would be for a "fake" multivariate 1 variable power series input for an integer to return the homogeneous component. E.g., for It's a slightly annoying difference, yes, but the model we are using does matter. |
@mantepse Do you think the current explanation is good? So you can still have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, perhaps we can get this in right now. @mantepse If you object (e.g., need more time to consider stuff), feel free to revert.
A nice while-we-are-at-it thing would be making the error message begin with a lower case letter and without a period/full-stop at the end to be consistent with Python's error message style. If you want to do this here, please go ahead and do it; you can reset the positive review after. Otherwise, it can be handled separately.
Fixes #39314. Adds an enhancement to power series that when given a tuple of values corresponding to the powers of the term gives you the coefficient for said term. Functionality is similar to how coefficients are retrieved for multivariable polynomials. Also added a test for the enhancement.
📝 Checklist
⌛ Dependencies