SetPartition return empty iterator instead of erroring out in degenerate cases
#37781
Conversation
|
Also, I'm not sure if a similar fix would be needed for the case when the second argument is a list. The element in the list should add up to the length of the set: sage: SetPartitions(range(4), [2, 1, 1])
Set partitions of {0, 1, 2, 3} with sizes in [2, 1, 1]
sage: SetPartitions(range(4), [2, 1])
...
ValueError: part must be an integer partition of 4We have the equivalence sage: sorted(list(SetPartitions(range(4), 3))) == sorted(list(SetPartitions(range(4), [2, 1, 1])))
TrueSo maybe the error above should also be replaced by an empty iterator. |
yes |
|
Documentation preview for this PR (built with commit 6683de7; changes) is ready! 🎉 |
There was a problem hiding this comment.
Let me also mention as a counterpoint, having the errors here are useful for catching logic bugs. So OrderedSetPartitions would error out for such clearly bad input.
Personally, I agree with your change here. However, it is something to consider.
Also, in case you haven't already, please check to make sure every place within Sage this is being called, the code isn't trying to catch the ValueError as program flow. (I don't think this is done anywhere, but we should check as it might not be covered by doctests.)
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
I agree that getting an empty iterator in this case would most likely be unintended.
The only other file where the keyword |
sagemathgh-37781: `SetPartition` return empty iterator instead of erroring out in degenerate cases <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Replaced the error raised by `SetPartition` in a degenerate case (see below) by an empty iterator, to match the behaviour of similar functions such as `Partitions` and `OrderedSetPartitions`. Old behaviour: ```Python sage: list( Partitions(3,length=4) ) [] sage: list( OrderedSetPartitions(range(3),4) ) [] sage: list( SetPartitions(range(3),4) ) ... ValueError: part must be <= len(set) ``` New behaviour ```Python sage: list( SetPartitions(range(3),4) ) [] ``` This solves issue sagemath#37643. ### 📝 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 accordingly. ### ⌛ 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#37781 Reported by: Nolord Reviewer(s): Travis Scrimshaw
Replaced the error raised by
SetPartitionin a degenerate case (see below) by an empty iterator, to match the behaviour of similar functions such asPartitionsandOrderedSetPartitions. Old behaviour:New behaviour
This solves issue #37643.
📝 Checklist
⌛ Dependencies