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

Speedup of Poset characteristic polynomial #35173

Merged
merged 5 commits into from
Mar 26, 2023

Conversation

trevorkarn
Copy link
Contributor

@trevorkarn trevorkarn commented Feb 22, 2023

📚 Description

Add special method for Moebius function computation with minimal element, add cached_method to HasseDiagram.bottom, change to sum over bottom_moebius_function

This drastically reduces the time required to compute characteristic polynomials. On commit fbb412787f, we see

sage: P = posets.SetPartitions(7)
sage: %time P.characteristic_polynomial()
CPU times: user 42.2 s, sys: 475 ms, total: 42.7 s
Wall time: 45.2 s
q^6 - 21*q^5 + 175*q^4 - 735*q^3 + 1624*q^2 - 1764*q + 720

With this commit we see:

sage: P = posets.SetPartitions(7)
sage: %time P.characteristic_polynomial()
CPU times: user 62.2 ms, sys: 1.83 ms, total: 64 ms
Wall time: 63.1 ms
q^6 - 21*q^5 + 175*q^4 - 735*q^3 + 1624*q^2 - 1764*q + 720

Fixes #34971

📝 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

None

@tscrim is a suggested reviewer.

…ent, add cached_method to HasseDiagram.bottom, change to sum over bottom_moebius_function
@trevorkarn
Copy link
Contributor Author

How do I add labels? This link https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels seems to say I should see a gear next to "Labels" on the right sidebar, but I don't see that. Am I doing something wrong?

@mkoeppe
Copy link
Member

mkoeppe commented Feb 22, 2023

I've sent you an invite to the Triage team, which has the necessary permissions

@trevorkarn
Copy link
Contributor Author

@mkoeppe Thanks!

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.

Other than the small changes, LGTM. Yet there is a slightly bigger change that would get you some even more speed if you chose to do it.

In the depth_first_search() in the backend, there is an optional reverse parameter that is not exposed through the Sage Digraph.depth_first_search. I would expose that and use that instead of neighbors=self.neighbors_in as then it will be using the Cython code.

src/sage/combinat/posets/hasse_diagram.py Outdated Show resolved Hide resolved
src/sage/combinat/posets/hasse_diagram.py Outdated Show resolved Hide resolved
@dimpase dimpase changed the title Speedup characteristic polynomial Speedup of Poset characteristic polynomial Feb 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2023

Codecov Report

Base: 88.59% // Head: 88.59% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (2d99bd7) compared to base (fbb4127).
Patch coverage: 94.73% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35173      +/-   ##
===========================================
- Coverage    88.59%   88.59%   -0.01%     
===========================================
  Files         2140     2140              
  Lines       396961   397289     +328     
===========================================
+ Hits        351677   351965     +288     
- Misses       45284    45324      +40     
Impacted Files Coverage Δ
src/sage/combinat/posets/posets.py 93.86% <50.00%> (-0.13%) ⬇️
src/sage/combinat/posets/hasse_diagram.py 98.81% <100.00%> (-0.08%) ⬇️
src/sage/plot/histogram.py 96.77% <0.00%> (-3.23%) ⬇️
src/sage/interfaces/qsieve.py 71.30% <0.00%> (-2.61%) ⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py 51.54% <0.00%> (-1.43%) ⬇️
src/sage/homology/matrix_utils.py 87.15% <0.00%> (-0.92%) ⬇️
src/sage/modular/quasimodform/element.py 99.14% <0.00%> (-0.86%) ⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py 94.09% <0.00%> (-0.79%) ⬇️
src/sage/combinat/colored_permutations.py 95.46% <0.00%> (-0.60%) ⬇️
src/sage/sets/integer_range.py 91.41% <0.00%> (-0.51%) ⬇️
... and 45 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@trevorkarn
Copy link
Contributor Author

In the depth_first_search() in the backend, there is an optional reverse parameter that is not exposed through the Sage Digraph.depth_first_search. I would expose that and use that instead of neighbors=self.neighbors_in as then it will be using the Cython code.

Doing this I get
sage: %time P.characteristic_polynomial()
CPU times: user 25.5 ms, sys: 817 µs, total: 26.3 ms
Wall time: 25.9 ms
q^6 - 21*q^5 + 175*q^4 - 735*q^3 + 1624*q^2 - 1764*q + 720

compared to
sage: %time P.characteristic_polynomial()
CPU times: user 63.5 ms, sys: 1.75 ms, total: 65.3 ms
Wall time: 64.8 ms
q^6 - 21*q^5 + 175*q^4 - 735*q^3 + 1624*q^2 - 1764*q + 720

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 2d99bd7

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.

Thanks. LGTM.

@vbraun vbraun merged commit 85b1e02 into sagemath:develop Mar 26, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 26, 2023
@trevorkarn trevorkarn deleted the char-poly-speedup branch May 8, 2023 21:31
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.

Speed up poset characteristic polynomial computation by caching Möbius function
6 participants