Opened 10 years ago
Closed 10 years ago
#10651 closed defect (fixed)
Make a good use of the EmptySetError in EnumeratedSets and Parent
Reported by: | nborie | Owned by: | nborie |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | categories | Keywords: | days28, empty set, EmptySetError |
Cc: | sage-combinat | Merged in: | sage-4.7.alpha3 |
Authors: | Nicolas Borie, Nicolas M. Thiéry | Reviewers: | Paul Zimmermann, Robert Bradshaw, Nicolas Borie, Nicolas M. Thiéry |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
As Florent Hivert gave extremely useful features about empty things, we should use it. The Following should returns an EmptySetError?
sage: S = Set([]) sage: S.an_element() --------------------------------------------------------------------------- StopIteration Traceback (most recent call last) ... StopIteration: sage:
Also, make the EmptySetError? being documented already in the structure of Parent. Improve the documentation of an_element and _an_element_ in Parent.
Attachments (1)
Change History (17)
comment:1 Changed 10 years ago by
- Cc hivert added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Keywords days28 added
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
Good point. I will ask today Nicolas what does he think about that... I don't really know the specifications for the Test Suite and how deal exactly with such corner case. Florent did this with FiniteEnumeratedSet? (The user know a priori that the set is finite...)
sage: FiniteEnumeratedSet([]) {} sage: FiniteEnumeratedSet([]).some_elements() <generator object _some_elements_from_iterator at 0xb433464>
I will give an answer shortly for that.
comment:5 Changed 10 years ago by
As this ticket fix a corner case and try to increase of the coherence for the code about sets, it have to verify all the specifications. Thanks you very much Robert for pointing this. It now return []. Even the behavior is the same, I don't want to fix a mathematical corner case by introducing a Spec corner case...
It should be ready for review now.
comment:6 Changed 10 years ago by
- Reviewers set to Paul Zimmermann
- Status changed from needs_review to positive_review
I confirm the problem is fixed, and all doctests still pass. Thus I give a positive review.
Paul
comment:7 Changed 10 years ago by
- Status changed from positive_review to needs_work
I think you should add an example which shows the EmptySetError
.
comment:8 Changed 10 years ago by
- Status changed from needs_work to needs_review
I just had two tests... Especially one which returns the EmptySetError?... Thanks all for your comments and suggestions.
comment:9 Changed 10 years ago by
- Status changed from needs_review to needs_work
- Work issues set to update documentation
after thought, the on-line documentation of S.an_element
should mention that if S
is
empty, an EmptySetError
is returned.
Paul
comment:10 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:11 Changed 10 years ago by
- Status changed from needs_review to positive_review
Looking good now.
comment:12 Changed 10 years ago by
- Milestone changed from sage-4.6.2 to sage-4.7
comment:13 Changed 10 years ago by
- Status changed from positive_review to needs_work
- Work issues update documentation deleted
You should change the commit message of the patch (using hg qrefresh -e
) to something sensible, making sure the ticket number appears on the first line of the commit message.
Changed 10 years ago by
comment:14 Changed 10 years ago by
- Cc nthiery added; hivert removed
- Description modified (diff)
- Reviewers changed from Paul Zimmermann to Paul Zimmermann, Robert Bradshaw, Nicolas Borie
- Status changed from needs_work to needs_review
- Summary changed from Small fix in `_an_element_from_iterator` in EnumeratedSets to Make a good use of the EmptySetError in EnumeratedSets and Parent
In order to fix more things in the same time, I update the description of this ticket. I folded the NT'reviewer from the sage-combinat queue, then I uploaded the patch with a better commit message.
I am very OK with the changes in the Parent structure from Nicolas Thiéry. I give a positive review on all changes touching the corresponding file. The first part of the patch still comes from me, so no review from me.
comment:15 Changed 10 years ago by
- Cc sage-combinat added; nthiery removed
- Reviewers changed from Paul Zimmermann, Robert Bradshaw, Nicolas Borie to Paul Zimmermann, Robert Bradshaw, Nicolas Borie, Nicolas M. Thiéry
- Status changed from needs_review to positive_review
And positive review on your part. Good to go!
comment:16 Changed 10 years ago by
- Merged in set to sage-4.7.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Technically, some_elements should return an iterable, not an iterator, so you can iterate over it more than once. The except clause should be changed to return [], not return iter([]), though with the empty case the behavior is the same.