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

new iterators over the partitions of an integer #38054

Merged
merged 11 commits into from
Jun 1, 2024

Conversation

dcoudert
Copy link
Contributor

As discussed in #37977, we implement new iterators over the partitions of an integer to get iterators generating partitions in increasing/decreasing lexicographic orders with partitions in ascending/descending orders.
We also implement methods yielding the next partition in a specific ordering.

📝 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

@dcoudert
Copy link
Contributor Author

The fastest is AccelAsc, but the difference is small.

sage: from sage.combinat.partitions import ZS1_iterator, ZS2_iterator, AccelDesc_iterator, AccelAsc_iterator
sage: %timeit sum(1 for _ in ZS1_iterator(10))
10.8 µs ± 29.4 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit sum(1 for _ in ZS2_iterator(10))
12 µs ± 108 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit sum(1 for _ in AccelDesc_iterator(10))
10.9 µs ± 7.62 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit sum(1 for _ in AccelAsc_iterator(10))
10.8 µs ± 73.1 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage:
sage: %timeit sum(1 for _ in ZS1_iterator(50))
47.8 ms ± 393 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit sum(1 for _ in ZS2_iterator(50))
52.1 ms ± 61.6 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit sum(1 for _ in AccelDesc_iterator(50))
47.4 ms ± 45.1 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit sum(1 for _ in AccelAsc_iterator(50))
46.4 ms ± 44.8 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

@dcoudert dcoudert requested review from dimpase and tscrim May 22, 2024 08:41
Copy link

github-actions bot commented May 22, 2024

Documentation preview for this PR (built with commit c5c4a3e; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@dimpase
Copy link
Member

dimpase commented May 22, 2024

linting errors?

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.

At least it seems like we should not use ZS2 over AccelAsc for increasing lex iteration by default. I am not opposed to keeping both implementations with some test for a sufficiently large n (say, between 50 and 100) showing the outputs are equal at every step.

I think we should add a prev method to partitions as well.

Other than that, I am good with this.

src/sage/combinat/partitions.pyx Show resolved Hide resolved
@dcoudert
Copy link
Contributor Author

At least it seems like we should not use ZS2 over AccelAsc for increasing lex iteration by default. I am not opposed to keeping both implementations with some test for a sufficiently large n (say, between 50 and 100) showing the outputs are equal at every step.

The orderings are not the same. Both generate partitions in lexicographic order, but the partitions of ZS2 are in descending order while the partitions of AccelAsc are in ascending order. So both might be of interest for some users.

I think we should add a prev method to partitions as well.

done.

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.

Thank you.

At least it seems like we should not use ZS2 over AccelAsc for increasing lex iteration by default. I am not opposed to keeping both implementations with some test for a sufficiently large n (say, between 50 and 100) showing the outputs are equal at every step.

The orderings are not the same. Both generate partitions in lexicographic order, but the partitions of ZS2 are in descending order while the partitions of AccelAsc are in ascending order. So both might be of interest for some users.

To me, a partition is always a weakly decreasing sequence (or a multiset), but yes, I agree that having the reverse order within the lists could be useful.

Just a few more minor things.

src/sage/combinat/partition.py Outdated Show resolved Hide resolved
src/sage/combinat/partitions.pyx Outdated Show resolved Hide resolved
src/sage/combinat/partitions.pyx Outdated Show resolved Hide resolved
src/sage/combinat/partitions.pyx Outdated Show resolved Hide resolved
src/sage/combinat/partitions.pyx Show resolved Hide resolved
src/sage/combinat/partitions.pyx Show resolved Hide resolved
@dcoudert
Copy link
Contributor Author

Thanks for the comments.

@tscrim
Copy link
Collaborator

tscrim commented May 28, 2024

The bot is showing some failures:

sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/groups/misc_gps/argument_groups.py  # 6 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/interfaces/gp.py  # 1 doctest failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/libs/pari/tests.py  # 2 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/lazy_series_ring.py  # 2 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/lazy_series.py  # 2 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/padics/padic_generic_element.pyx  # 2 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/qqbar.py  # 1 doctest failed
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/stats/basic_stats.py  # 2 doctests failed

I am not sure how many of them are real failures due to the changes here. Some seem unrelated from the log.

@dcoudert
Copy link
Contributor Author

This seems unrelated. On my side, I don't observe these errors. I only see 1 warning

sage -t --long --warn-long 25.3 --random-seed=286735480429121101562228604801325644303 src/sage/interfaces/gp.py
**********************************************************************
File "src/sage/interfaces/gp.py", line 799, in sage.interfaces.gp.Gp.new_with_bits_prec
Warning: Variable 'pi_def' referenced here was set only in doctest marked '# needs sage.libs.pari sage.symbolic'
    pi_def.precision()
    [157 tests, 2.40 s]

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 for checking. I also saw today that that group of errors was reported on sage-release. So positive review.

@dcoudert
Copy link
Contributor Author

Thank you for the review.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 29, 2024
    
As discussed in sagemath#37977, we implement new iterators over the partitions
of an integer to get iterators generating partitions in
increasing/decreasing lexicographic orders with partitions in
ascending/descending orders.
We also implement methods yielding the next partition in a specific
ordering.


### 📝 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.
- [x] 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#38054
Reported by: David Coudert
Reviewer(s): David Coudert, Travis Scrimshaw
@vbraun vbraun merged commit 9a17b34 into sagemath:develop Jun 1, 2024
14 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 1, 2024
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.

5 participants