Opened 11 years ago
Closed 10 years ago
#10776 closed defect (fixed)
poset top() function breaks when top element has boolean value False
Reported by: | niles | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | combinatorics | Keywords: | beginner, poset, top |
Cc: | niles, novoselt | Merged in: | sage-4.7.alpha4 |
Authors: | Niles Johnson | Reviewers: | Andrey Novoseltsev |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Posets determine whether there is a top element by converting the element returned by ._hasse_diagram.top()
to a boolean. Instead, the function should check whether the returned value is None
.
Here is the current definition of .top()
:
def top(self): """ Returns the top element of the poset, if it exists. EXAMPLES:: sage: P = Poset({0:[3],1:[3],2:[3],3:[4,5],4:[],5:[]}) sage: P.top() is None True sage: Q = Poset({0:[1],1:[]}) sage: Q.top() 1 """ hasse_top = self._hasse_diagram.top() if hasse_top: return self._vertex_to_element(hasse_top) else: return None
And here is an example showing how it fails:
sage: p = Poset([[0],[]]); p Finite poset containing 1 elements sage: p.list() [0] sage: p.top() sage: p.top() is None True sage: p._hasse_diagram.top() 0 sage: p._hasse_diagram.top() is None False
Attachments (1)
Change History (10)
comment:1 Changed 11 years ago by
- Cc novoselt added
- Milestone set to sage-4.7
comment:2 Changed 11 years ago by
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 11 years ago by
Looks good, but it would be nice to mention ticket number in the TESTS ;-)
comment:4 in reply to: ↑ 3 Changed 11 years ago by
Replying to novoselt:
Looks good, but it would be nice to mention ticket number in the TESTS ;-)
Well, actually I've thought about this before -- I think there was a time when the developer guide recommended it. But when I looked into this a few months ago, I couldn't find the recommendation any more, and someone convinced me to drop it with something like the following: Wouldn't it be strange and pointless if, many years from now, the Sage source code were filled with references to ticket numbers that are meaningless to most users?
Do you really think there's a compelling reason to mention the ticket?
comment:5 follow-up: ↓ 6 Changed 11 years ago by
The fifth bullet in http://sagemath.org/doc/developer/trac.html#section-review-patches
I don't have a strong opinion on it and was only mentioning it since it is required. (I have also realized that I never marked such doctests with the inline comment and I am pretty sure not all bugs that I have reviewed used them...)
As for meaningless references, perhaps it isn't very relevant since most of such "marked" tests should go into TESTS section and not EXAMPLES, so only developers will look at them. On the other hand, I didn't ever have a need to follow these references. But that may be because I am not a sufficiently active developer ;-)
comment:6 in reply to: ↑ 5 Changed 11 years ago by
Replying to novoselt:
The fifth bullet in http://sagemath.org/doc/developer/trac.html#section-review-patches
Well, I stand corrected -- somehow I can't find that list when I'm looking for it; probably because I associate it with reviewing patches rather than creating them.
...
Oh, how embarrassing: checking back, it seems that I *wrote* that line (#9771). So the new patch adds the ticket number :)
thanks for keeping me honest,
Niles
comment:7 Changed 11 years ago by
- Reviewers set to Andrey Novoseltsev
- Status changed from needs_review to positive_review
Well, that was a long time ago ;-) Positive review!
comment:8 Changed 11 years ago by
:) thanks!
comment:9 Changed 10 years ago by
- Merged in set to sage-4.7.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
patched; written to parallel the code for
.bottom()
.