#18929: Include igraph library
-------------------------------------+-------------------------------------
Reporter: borassi | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.8
Component: packages: | Resolution:
optional | Merged in:
Keywords: igraph library | Reviewers: Nathann Cohen
Authors: Michele Borassi | Work issues:
Report Upstream: N/A | Commit:
Branch: | 24920b6ffa69b6e92efabf359970a96464912d8e
u/borassi/include_igraph_library | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by ncohen):
Helooooooo Michele,
I finished reviewing this ticket again. I'm sorry but if there is one
thing I cannot stand, it is to see heaps of code meant to deal with edge
labels/multile edges/loops/weights. Dozens of line which do nothing but
wrap junk into another kind of junk.
Well.
I added a commit at public/18929 which does the following modifications,
which you can of course accept/discuss/reject:
- documentation of `igraph` input -- I removed the end of the sentence
about what is done with weights/names/etc. It is not said for other
formats and it is how it is expected to work. I thought that the shortest
was the best.
- removed some trailing whitespaces
- changed the message in which you mentionned a 'network'. Graphs are..
Well, nobody exactly knows. Depending on who you talk to, when you say "a
graph" people hear "social network graph", "router graph", "planar graph",
"vertex-transitive graph", "cayley graph", and many others. Networks do
exist, but that would not make *any* sense for some of those guys, so I
stuck to the 'graph' terminology.
- I "compressed" a bit the copy/paste meant to handle names, by using the
relabel function. For this I had to change a line in
`GenericGraph.relabel` related to `_boundary`, but this `#O&O#&&^@)#%*(`
is meant to be removed in #17462 anyway.
- Try to not use `except Exception` if you can avoid it. That's too large.
If there is a typo in the line before it, it will catch this too and you
don't want that. Also, try to keep as little as possible in a try/except.
Finally, I have to say that I can't bring myself to give a positive review
to this latest commit. Much of what it does is, to me, meant to be done by
the user. We cannot adapt to the many forms he wants his data to take in
the igraph graph, so I think that the best we should do is ... "keep it
simple and reliable". Guessing that 'label' is a special key in the
dictionary is (to me) going way too far, and I am tempted to do the same
for weights. Of course their labels *have* to be dictionaries and ours can
be anything. That's unpleasant, but turning our label into a
`{'label':label}` in their data structure, and setting their dictionary to
be our edge label is to me the best we can do.
So yes, if you apply translation several times you will not converge.
Honestly, I do not care at all.
Sooooo, sorry for that. You have several ways out:
- Get this ticket reviewed by somebody else (I will not force my view, I
just won't review it myself -- no way)
- Remove the commits you added since my last 'positive review' so that
this ticket can get merged, and move those label commits to another
ticket.
As you wish.
Have fun,
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18929#comment:25>
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.
