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

improve performance of stream cauchy invert #35338

Conversation

mantepse
Copy link
Collaborator

📚 Description

Replace loops by sums with lazy assignments and provide a dedicated get_coefficient for Stream_cauchy_invert

Fixes #34553

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mantepse
Copy link
Collaborator Author

Warning: there is another commit, which removed Stream_cauchy_invert.get_coefficient: 814aa7c

I am not sure what to make of this. Also, in the get_coefficient here, it seems to me that at least isinstance(self._series, Stream_exact) and not self._series._constant could be done in __init__.

We would have to check whether the modifications here are really an improvement.

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023

Codecov Report

Patch coverage: 93.08% and project coverage change: -0.01 ⚠️

Comparison is base (5330147) 88.61% compared to head (3ef7684) 88.61%.

❗ Current head 3ef7684 differs from pull request most recent head 31ca686. Consider uploading reports for the commit 31ca686 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35338      +/-   ##
===========================================
- Coverage    88.61%   88.61%   -0.01%     
===========================================
  Files         2148     2148              
  Lines       398855   398913      +58     
===========================================
+ Hits        353438   353488      +50     
- Misses       45417    45425       +8     
Impacted Files Coverage Δ
src/sage/rings/lazy_series_ring.py 89.77% <ø> (ø)
src/sage/rings/lazy_series.py 92.35% <84.21%> (-0.08%) ⬇️
src/sage/data_structures/stream.py 96.96% <94.28%> (-0.51%) ⬇️

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tscrim
Copy link
Collaborator

tscrim commented Mar 24, 2023

We got rid of it because we realized that the Cauchy inverse always had to compute things in order, so it was always dense. Thus, any changes need to be in iterate_coefficients().

@mantepse
Copy link
Collaborator Author

Ah, excellent that you remember / are pointing this out! I don't know yet when I can continue working on this, but all your comments so far are very helpful, thank you!

@tscrim
Copy link
Collaborator

tscrim commented Mar 25, 2023

I will try to look at this again soon too to try and see what speedups can be done, but it will probably not be a high priority for me.

@mkoeppe mkoeppe removed this from the sage-10.0 milestone May 4, 2023
@github-actions
Copy link

Documentation preview for this PR (built with commit 1d52328; changes) is ready! 🎉

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM.

@vbraun vbraun merged commit d0759e8 into sagemath:develop Sep 1, 2023
11 of 12 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 1, 2023
@mantepse mantepse deleted the u/mantepse/improve_performance_of_stream_cauchy_invert branch September 1, 2023 21:04
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.

improve performance of Stream_cauchy_invert
5 participants