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

DisjointSet ignores errors #38939

Open
videlec opened this issue Nov 8, 2024 · 5 comments · May be fixed by #38944
Open

DisjointSet ignores errors #38939

videlec opened this issue Nov 8, 2024 · 5 comments · May be fixed by #38944
Labels

Comments

@videlec
Copy link
Contributor

videlec commented Nov 8, 2024

Because DisjointSet is a cython class, some care has to be taken with respect to functions that contain python code that could possibly raise an error. For example, the function union does contain python code that could possibly raise an error, but the error is not raised

sage: d = DisjointSet('abcde')
sage: d.union('a', 'f')
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
KeyError: 'f'
Exception ignored in: 'sage.sets.disjoint_set.DisjointSet_of_hashables.union'
Traceback (most recent call last):
  File "<ipython-input-8-206901714f0d>", line 1, in <module>
KeyError: 'f'
@videlec videlec added the t: bug label Nov 8, 2024
@DaveWitteMorris
Copy link
Member

Please clarify why this is a bug. I think raising an error is correct here (since 'f' is not one of the sets).

@videlec
Copy link
Contributor Author

videlec commented Nov 8, 2024

The bug is that the error is not raised. Isn't it explicit enough?

@DaveWitteMorris
Copy link
Member

OK, I was confused. (My bad, but github issues usually include both the expected behavior and the actual behavior, and I was confused about both in this case.) If I understand correctly, currently the code prints a message but does not raise an error, but you (like me) think it should raise an error.

@videlec
Copy link
Contributor Author

videlec commented Nov 9, 2024

Indeed. The code prints that an error has occurred and been ignored. And I think it should raise an error.

@dcoudert
Copy link
Contributor

dcoudert commented Nov 9, 2024

method union is currently defined with a noexcept. We can change that to except *. I will push a possible fix. Let's hope it will not break too many tests.

@dcoudert dcoudert linked a pull request Nov 9, 2024 that will close this issue
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this issue 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
vbraun pushed a commit to vbraun/sage that referenced this issue Nov 14, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants