#19197: LatticePoset: add breadth()
-------------------------------------+-------------------------------------
       Reporter:  jmantysalo         |        Owner:
           Type:  enhancement        |       Status:  needs_review
       Priority:  major              |    Milestone:  sage-6.9
      Component:  combinatorics      |   Resolution:
       Keywords:                     |    Merged in:
        Authors:  Jori Mäntysalo     |    Reviewers:
Report Upstream:  N/A                |  Work issues:
         Branch:                     |       Commit:
  u/jmantysalo/latticeposet__add_breadth__|  
33610ad2c4327679b8dfc1d3af445fe4d9080e4a
   Dependencies:                     |     Stopgaps:
-------------------------------------+-------------------------------------

Comment (by ncohen):

 Hello Jori,

 Here is a first-pass review while on the bus.

 - I was very surprised to see a 'distance' argument in the depth-first-
 search
   function. You do not compute the 'distance' when running a
   depth-first-search. At least not the usual notion of distance in graph
   theory. We should probably remove it, as it is *highly* dangerous to
 advertise
   it this way `O_o`

 - In your code, please use breadth-first-search instead.

 - Definition of 'elems': this line often test for containment in
 'too_close'. Do
   not run containment tests in a list. use a 'set' for that: containment
 is
   faster.

 - Anyway, you probably should do this differently, i.e. with
   `breadth_first_search(report_distance=True)` while filtering 'too close'
   elements according to their distance.

 - I still believe that it would be faster to use
   'subsets_with_hereditary_property' inside of your code. It would also
 make the
   code slightly clearer, as the test you perfom on each antichain would be
 an
   independent function.

 - Why do you deal with breadth 2 differently from the rest? That could
 mean a
   couple more operations, but compared to everything else that should not
 mean
   much.

 Besides that, congratulations for this algorithm. It works, and it works
 well.

 Nathann

--
Ticket URL: <http://trac.sagemath.org/ticket/19197#comment:15>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica, 
and MATLAB

-- 
You received this message because you are subscribed to the Google Groups 
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.

Reply via email to