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

no longer ignore errors in method union of DisjointSet #38944

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

dcoudert
Copy link
Contributor

@dcoudert dcoudert commented Nov 9, 2024

Fixes #38939.

This PR changes the declaration of method union from noexcept to except * to properly raise errors.

📝 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 dcoudert self-assigned this Nov 9, 2024
@dcoudert dcoudert requested a review from videlec November 9, 2024 10:14
Copy link

github-actions bot commented Nov 9, 2024

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

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 pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
…intSet`

    
Fixes sagemath#38939.

This PR changes the declaration of method `union` from `noexcept` to
`except *` to properly raise errors.

### 📝 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.
- [ ] 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#38944
Reported by: David Coudert
Reviewer(s): Travis Scrimshaw
@gmou3
Copy link
Contributor

gmou3 commented Nov 15, 2024

Can you please report the time it takes to run

%time matroids.CompleteGraphic(8).circuits()

before and after the change?

@dcoudert
Copy link
Contributor Author

The slowdown seems rather limited.

Before:

sage: %timeit matroids.CompleteGraphic(3).circuits()
53.3 µs ± 68.4 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit matroids.CompleteGraphic(4).circuits()
118 µs ± 526 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit matroids.CompleteGraphic(5).circuits()
1.23 ms ± 9.29 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit matroids.CompleteGraphic(6).circuits()
23.2 ms ± 92.2 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit matroids.CompleteGraphic(7).circuits()
525 ms ± 3.07 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit matroids.CompleteGraphic(8).circuits()
13.6 s ± 59.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

With this PR:

sage: %timeit matroids.CompleteGraphic(3).circuits()
56.6 µs ± 111 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit matroids.CompleteGraphic(4).circuits()
124 µs ± 756 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit matroids.CompleteGraphic(5).circuits()
1.29 ms ± 24.1 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit matroids.CompleteGraphic(6).circuits()
23.4 ms ± 186 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit matroids.CompleteGraphic(7).circuits()
543 ms ± 12.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit matroids.CompleteGraphic(8).circuits()
13.9 s ± 59.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

@gmou3
Copy link
Contributor

gmou3 commented Nov 15, 2024

Thanks for running the tests.

I suppose the slowdown can be ignored if the error handling is deemed to be of some value.

@vbraun vbraun merged commit 5c957f8 into sagemath:develop Nov 16, 2024
22 of 24 checks passed
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.

DisjointSet ignores errors
4 participants