#19249: Interface with TdLib
-------------------------------------+-------------------------------------
Reporter: llarisch | Owner:
Type: enhancement | Status: needs_info
Priority: minor | Milestone: sage-6.10
Component: packages: | Resolution:
experimental | Merged in:
Keywords: | Reviewers: Nathann Cohen
Authors: | Work issues:
Report Upstream: N/A | Commit:
Branch: | f438342a8fe42f817ef8efac79b88309e9412768
u/llarisch/interface_with_tdlib | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by ncohen):
* status: needs_review => needs_info
* reviewer: => Nathann Cohen
Comment:
Helloooooooooo Lukas,
Here is another review:
- From the url at which you placed your tarball I found your website, and
this
page [1] really looks like "TdLib's official page". Could you add this
url at
the end of the 'description' section of the SPKG.txt file?
- It would be cool if your tarball contained a 'license' (or 'copying')
file
asserting that it is licensed under the GPL2.
- I do not understand why you need to copy/paste sage_tdlib.cpp into your
tarball if you want it to compile. Could you please explain me? In no
other
package do we need to do such a thing. Usually the tarball works by
itself,
and we have in Sage some interface code that is linked with it.
- One such instance is the interface with boost. You will find it in the
following files:
- sage/graphs/base/boost_graph.pyx
- sage/graphs/base/boost_graph.pxd
- sage/graphs/base/boost_interface.cpp
- Your 'spkg-install' file copies files that you removed from your tarball
(i.e. ./.libs/*)
- I do not understand what exactly this warning means. Could you tell me?
`^^;`
{{{
+#!!!!!! NOTICE !!!!!!!!
+#Sage vertices have to be named by unsigned integers
+#Sage bags of decompositions have to be lists of unsigned integers
+#!!!!!!!!!!!!!!!!!!!!!!!!!!
}}}
- Could you strip 'cython_' from the beginning of the functions you
define?
- The code of `get_width` can be replaced by:
{{{
return max(len(x) for x in T)-1
}}}
- It is slightly incorrect to have your class `TreeDecomposition` inherit
from
`Graph`, while having a function like `get_width` rely on the vertices'
labels
(you assume that they are sets). Indeed, the vertices of a graph can be
relabeled, which would break this function. I don't think that you need
this
class, to be honest.
- We have something like `for(auto i: something)` in python. You can
rewrite
{{{
+ V_python = G.vertices()
+ for i in range(0, len(V_python)):
+ V.push_back(V_python[i])
}}}
As
{{{
+ for v in G:
+ V.push_back(v)
}}}
(it occurs several times in your code)
- Similarly (don't scream -- I know it can hurt when you see it for the
first
time) you can write this in Python:
{{{
a,b = 1,2
}}}
Which means that this also works
{{{
for u,v,l in G.edges():
E.push_back(u)
E.push_back(v)
}}}
Note that if you don't need the labels, then this also works:
{{{
for u,v in G.edges(labels=False):
E.push_back(u)
E.push_back(v)
}}}
- Some of your 'TEST' and 'EXAMPLE' tags are not indented as they should
(4
spaces).
Nathann
[1] http://www.tdi.informatik.uni-frankfurt.de/~lukas/
--
Ticket URL: <http://trac.sagemath.org/ticket/19249#comment:33>
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.
