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 coefficients(a::AbsPowerSeriesRingElem) #1811

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Conversation

lgoettgens
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.03%. Comparing base (efb6ceb) to head (73f0fdc).
Report is 16 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1811   +/-   ##
=======================================
  Coverage   88.03%   88.03%           
=======================================
  Files         119      119           
  Lines       30057    29995   -62     
=======================================
- Hits        26460    26406   -54     
+ Misses       3597     3589    -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks plausible. Of course no test, so I am going to trust that you need this and verified it works "as expected" (whatever that means)

@thofma
Copy link
Member

thofma commented Sep 27, 2024

@fieker does that make sense?

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Sep 27, 2024

Ok, so what I wanted to add is a function that gives me all coefficients (as coeff does for a single one), but skipping the infinite sequence of zeros in the end.
If there is any other clean way to get this, I am happy with using that instead as well.

@fieker
Copy link
Contributor

fieker commented Sep 27, 2024

you need to be thinking about what you want: you want the leading zeros? If the element has a valuation? @lgoettgens what do you need/ want? I think for polys we allow access past the degree - to get 0, for power series (and Laurent) even going to -infty would make sense

@lgoettgens
Copy link
Collaborator Author

I don't really care about leading zeros.
I think having coefficients(a)[i] == coeff(a, i-1) work for all indices of coefficients(a) would be nice (this is as it is for polys).

@lgoettgens
Copy link
Collaborator Author

Bump

@thofma
Copy link
Member

thofma commented Oct 1, 2024

It is not clear to me that this right thing. length is not part of the interface and exporting pol_length was a mistake in the first place.

A faithful representation of the power series would need to include the coefficients up precision(x) - 1.

@lgoettgens
Copy link
Collaborator Author

That's true. I adapted the function an tests accordingly.

@fingolfin
Copy link
Member

Off-topic, but: this function looks to me like another example of an extrep function as proposed in oscar-system/Oscar.jl#4151. Which then raises the natural question: do we also have a reverse that takes such a coefficient list and turns it back into a series? E.g. a == parent(a)(coefficients(a)) ?

@thofma
Copy link
Member

thofma commented Oct 2, 2024

@fieker this now goes up to precision(x) -1. Are you happy with this?

@lgoettgens
Copy link
Collaborator Author

Unfortunately, due to branch names being weird, this has to get force-merged. @fingolfin @thofma could you do the favor?

@thofma thofma merged commit 6227ca0 into master Oct 7, 2024
30 checks passed
@thofma thofma deleted the lgoettgens-patch-1 branch October 7, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants