Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#2519 closed enhancement (fixed)

[with patch; positive review] Add support for posets, semi-lattices, etc. to Sage

Reported by: saliola Owned by: saliola
Priority: major Milestone: sage-3.0.2
Component: combinatorics Keywords:
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description


Attachments (2)

posets.2008-04-23.patch (81.0 KB) - added by saliola 6 years ago.
Initial support for finite posets. (Patch against sage-3.0.)
posets.2008-05-15.patch (17.9 KB) - added by saliola 6 years ago.
Apply both patches in order!

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by saliola

Initial support for finite posets. (Patch against sage-3.0.)

comment:1 Changed 6 years ago by saliola

  • Summary changed from Add support for posets, semi-lattices, etc. to Sage to [with patch; needs criticism] Add support for posets, semi-lattices, etc. to Sage

comment:2 Changed 6 years ago by mabshoff

  • Milestone changed from sage-feature to sage-3.0.1
  • Summary changed from [with patch; needs criticism] Add support for posets, semi-lattices, etc. to Sage to [with patch; needs review] Add support for posets, semi-lattices, etc. to Sage

comment:3 follow-up: Changed 6 years ago by rlm

  • Summary changed from [with patch; needs review] Add support for posets, semi-lattices, etc. to Sage to [with patch; some suggestions] Add support for posets, semi-lattices, etc. to Sage

Some comments:

  1. Lattice should be LatticePoset or something similar. Same comment for Chain, Antichain, Pentagon and Diamond.
  1. This is a great example of good code. The docstrings are generous and everything is well tested. Nice work!


comment:4 in reply to: ↑ 3 Changed 6 years ago by saliola

  • Summary changed from [with patch; some suggestions] Add support for posets, semi-lattices, etc. to Sage to [with patch; needs review] Add support for posets, semi-lattices, etc. to Sage
  1. Lattice should be LatticePoset or something similar. Same comment for Chain, Antichain, Pentagon and Diamond.

Done. As a remark: we eventually want to create a poset database similar to the graph
database. So these poset examples will be moved into the database.

  1. This is a great example of good code. The docstrings are generous and everything is well tested. Nice work!

Thank you!

There are a few other differences between the original patch and the new patch:

1. Added "or equal to" to the docstrings for is_gequal and is_lequal.

2. I redefined the show methods for FinitePoset and HasseDiagram. Before it
was just show = plot, but I since realized this is wrong: show should show the
Graphics object returned by the plot method.

3. Added the methods interval, closed_interval and open_interval to the FinitePoset
class. These already existed for the HasseDiagram class; so the new methods
just expose these methods from the FinitePoset class. It was an oversight not
to have done this originally.

4. I slightly modified HasseDiagram.antichains to return the empty list as an
antichain. And added tests.

comment:5 follow-up: Changed 6 years ago by rlm

Regarding show versus plot, you don't even really need a show: most Sage objects give LaTeX when you call show, and if you do sage: P.plot(), the graphics object will appear.

Also, it looks like you need both patches in order, right?

Changed 6 years ago by saliola

Apply both patches in order!

comment:6 in reply to: ↑ 5 Changed 6 years ago by saliola

Replying to rlm:

Regarding show versus plot, you don't even really need a show: most Sage objects give LaTeX when you call show, and if you do sage: P.plot(), the graphics object will appear.

I'll leave it as is, as this is how it is implemented for Graphs/DiGraphs?.

Also, it looks like you need both patches in order, right?

Yes, I patched against the previous patch. I've corrected what I wrote.

I can provide a single patch instead if that is easier.

comment:7 Changed 6 years ago by rlm

  • Summary changed from [with patch; needs review] Add support for posets, semi-lattices, etc. to Sage to [with patch; (conditional) positive review] Add support for posets, semi-lattices, etc. to Sage

As long as it applies cleanly and passes all tests...

comment:8 Changed 6 years ago by mabshoff

  • Milestone changed from sage-3.0.3 to sage-3.0.2
  • Summary changed from [with patch; (conditional) positive review] Add support for posets, semi-lattices, etc. to Sage to [with patch; positive review] Add support for posets, semi-lattices, etc. to Sage

With both patches applied the doctests pass. Two comments:

  • sage/combinat/posets/init.py is zero bytes, so the patch doesn't create it. I fixed that manually
  • these patches are regular diffs and not mercurial patches, so I ended up with the formal credit when applying them. I did however state in the changelog:
    changeset:   9746:0f14afe7ba00
    tag:         tip
    user:        mabshoff@sage.math.washington.edu
    date:        Thu May 22 16:52:19 2008 -0700
    summary:     Apply patch by Peter Jipsen and Franco Saliola: Add support for posets, semi-lattices, etc
    
    changeset:   9745:0716b13a9e12
    user:        mabshoff@sage.math.washington.edu
    date:        Thu May 22 16:52:08 2008 -0700
    summary:     Apply patch by Peter Jipsen and Franco Saliola: Add support for posets, semi-lattices, etc.
    

Cheers,

Michael

comment:9 Changed 6 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.0.2.rc0

comment:10 Changed 5 years ago by nthiery

  • Cc sage-combinat added; jipsen@… removed
Note: See TracTickets for help on using tickets.