Opened 10 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)

trac_10776_poset_top.patch (985 bytes) - added by niles 10 years ago.
add ticket number in test

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 years ago by novoselt

  • Cc novoselt added
  • Milestone set to sage-4.7

comment:2 Changed 10 years ago by niles

  • Authors set to Niles Johnson
  • Status changed from new to needs_review

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

comment:3 follow-up: Changed 10 years ago by novoselt

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

comment:4 in reply to: ↑ 3 Changed 10 years ago by 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?

comment:5 follow-up: Changed 10 years ago by 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 ;-)

Changed 10 years ago by niles

add ticket number in test

comment:6 in reply to: ↑ 5 Changed 10 years ago by 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

comment:7 Changed 10 years ago by novoselt

  • 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 10 years ago by niles

:) thanks!

comment:9 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.