#18564: Boost Edge Connectivity
-------------------------------------+-------------------------------------
Reporter: borassi | Owner: borassi
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.8
Component: graph theory | Resolution:
Keywords: Boost, | Merged in:
connectivity | Reviewers:
Authors: Michele Borassi | Work issues:
Report Upstream: N/A | Commit:
Branch: | b513cf9f6a52e1dae5805cb622da9481069defc3
u/borassi/boost_edge_connectivity | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by borassi):
* status: needs_work => needs_review
Comment:
Hello,
I have tried to correct your problems. More details below.
Best,
Michele
> - There is a weird 'pass' in boost_graph.pxd
Removed!
>
> - `#clang c++` is not useful in Sage's source
Then I am afraid I have not understood your comment 20... Where should I
have put it? In any case, I removed it.
>
> - You should add your new file to the doc index
> (http://doc.sagemath.org/html/en/developer/sage_manuals.html#adding-a
-new-file)
Sorry, you have already told me to do so, but since I had to take many
things into account, I forgot. Now I have done it!
>
> Also, the header of your new module should describe what it implements
I have added the list of methods implemented, as in `generic_graph`. Is
this what you meant?
>
> - This will make you notice (Sphinx will scream) that whatever follows a
'::'
> should be indented.
Done!
>
> - A module's documentation is no place for 'todo'. If you want to work
on
> something, create a trac ticket.
Ok!
>
> - Why a static 'from_sage_graph' function instead of a constructor
accepting a
> graph as an input?
Sigh, you are right... Done!
>
> - `sig_on/sig_off` -- these markers are used so that the code can be
interrupted
> while some C code is being run. Why would you put them around functions
which
> end instantaneously, like `add_edge/add_vertex`?
Ups, I thought they were used to handle segmentation faults... My mistake!
>
> - The variable 'i' is also an integer, and you should define it as 'cdef
int' so
> that your Cython code will be better optimised.
I didn't define it as a cdef because that was not the bottleneck of the
algorithm, and I chose readability instead of efficiency. However, now I
used cdef.
>
> - Why would you define `edge_connectivity_boost_*` functions in
> `generic_graph_pyx` instead of doing it in the `graph_boost` file? You
can
> import them if needed.
Because, in my opinion, it could be better to keep the Boost
implementation separated from the Boost algorithms (since soon there will
be many of them), and to put the algorithms in the files where they will
be used. Anyway, I moved it back!
>
> - Why did you move the LP out of the current `edge_connectivity`
function? Can't
> you leave if where it is, and at the beginning of the file call the
boost
> functions if this is what the user wants?
Because I do not really like functions that are "too long", and I thought
that some code used with Boost could be re-used by the LP algorithm,
simplifying the function. Anyway, I have returned to the old version.
>
> - When you submit a branch, always look at the final 'diff'. You can see
it by
> clicking on the (green) branch's name on its ticket, and it will tell
you what
> the reviewer sees and how he will have to figure out what your code
> does. Everything that is not crystal clear for you is not crystal clear
for
> him.
Now the diff should be cleaner. Is there any way to see the diff before
pushing my changes (e.g. git diffall or something like that)?
> Finally: please report the bug you found to boost. They must fix it.
I will add the bug to the Boost trac as soon as the Boost trac server
(svn.boost.org/) starts to work again. If it doesn't, in a few days I will
try to find other ways to contact Boost developers.
--
Ticket URL: <http://trac.sagemath.org/ticket/18564#comment:30>
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.
