poset top() function breaks when top element has boolean value False

[niles]

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

[niles]

patched; written to parallel the code for .bottom().

[novoselt]

Looks good, but it would be nice to mention ticket number in the TESTS ;-)

[niles]

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?

[novoselt]

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 ;-)

[niles]

add ticket number in test

[niles]

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

[novoselt]

Well, that was a long time ago ;-) Positive review!

[niles]

:) thanks!