New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interface with TdLib #19249
Comments
Branch: u/llarisch/interface_with_tdlib |
New commits:
|
This comment has been minimized.
This comment has been minimized.
Commit: |
comment:4
Hello, This looks very nice indeed, but because I expect the review to be rather long in this case, it would probably be best to start by exposing a reduced amount of features from your library. A general function to compute the treewidth exactly, for instance, would be ideal as we would be able to compare its output with what we already have (which is slow). For a start, I guess that some things should be done directly in your library: is it necessary to have Sage apply a patch to it? Is there a reason why what your patch does cannot already be included in your code? Your makefile appears to be manual, and it would probably be best to have it created by autotools. I had to learn this not so long ago and I found it rather painful to learn, but the purpose is to make sure that your code will compile everywhere, so that you do not need to test every platform manually. I can probably help with that, as well as send you a pdf book that helped me a lot while learning it. Finally, I am not sure if you are aware that you can have C instructions in a 'def' function of a cython file. I see that you have several Is your code available on some web page? Is there a public repository for it? Are you the only author? Are there theoretical publications related to it? This kind of information would be interesting to us, and is expected to be found in a SPKG.txt file. http://doc.sagemath.org/html/en/developer/packaging.html#the-spkg-txt-file Regards, Nathann |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:6
While testing my version of an exact algorithm for treewidth against the sage version, i noticed that the sage version doesnt work: it is NOT sufficient to just consider the neighbourhood of computed cut. Counterexample: G = Graph() But G has treewidth 2. (you possibly have to change the vertex labeling to see the error). |
comment:7
Right, this graph clearly has width 2 |
comment:8
Replying to @nathanncohen:
And the error can be arbitrary bad. One can construct a graph, such that a cut is not connected in a graph and has to be included in a bag of a tree decomposition. Here it is {0,4,8}. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:10
I have to instantiante template functions and convert a "sage graph" to a boost graph.
Is there a minimal example for that? The "tdlib"-part of the documentation has failed to build (UNABLE TO IMPORT MODULE): Is there some import-statement missing? What can be the reasons (its not the content of the *.pyx-file)? |
comment:11
I still believe that you can do it without patching your code. Let your code deal with boost graphs only, and let Sage only call it on Boost instances (see sage/graphs/base/boost_graph.pyx). This issue should not be a reason to patch your library.
All over internet. One that was done recently for Sage is there:
Probably something like that: Nathann |
comment:12
Or, otherwise, because you should import your module like others, as it is done in all.py? |
comment:13
Is there a way to generate a boost graph with an appended struct? I've seen that ctypedef BoostGraph[vecS, vecS, undirectedS, vecS, EdgeWeight] BoostVecWeightedGraph exists, but I would need: ctypedef BoostGraph[vecS, vecS, undirectedS, vecS, X] BG, such that X is a vertex property. Is there a (not too complicated) way to realize this? |
comment:14
I do not know of one. |
comment:15
Replying to @nathanncohen:
In this case, the patches are inevitable (treedecompositions are boost graphs with a set as a vertex property). I could merely displace some graph conversion stuff into the library.. Does automake cp & rm files automatically? If this is the case: which flags have to be passed to autoconfig on the sage side in spkg-install? So far:
|
comment:16
Yes. Your library has no reason to be able to handle Sage graph, so just write Sage code to call it with the kind of graph that it expects.
I do not understand this question. Nathann |
comment:17
Hello,,
Thank you very much for this bug report. It has been fixed in #19358 (needs_review). Nathann |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:65
Err.. Sorry with the mess with the git branch. It should now be in a correct state, with just a merging operation atop of your branch and a commit meant to clean the doc/code. If you still have no problem with it, please change the ticket's status to Thanks, Nathann |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:67
Replying to @nathanncohen:
The current state is already |
comment:68
That's only because I am an idiot. I meant 'positive_review'. |
comment:69
In build/pkgs/tdlib/type, the package type is set to |
comment:70
There are still some problems with the trivial cases in the sage algorithm for treewidth:
|
comment:71
I think I fixed the second case in the commit I added here. The empty graph may also return 1, however. It has to be fixed but not necessarily in this ticket, so we can merge it anyway and fix that elsewhere. Nathann |
comment:73
I think you should have removed the whitespace changes to |
comment:74
|
comment:75
Sorry, I was careless. 'optional' flags were missing in the second file, and the behaviour was sometimes wrong in the first one. Lukas, as previously could you double check my last commit and change this ticket's status if you agree with it? Nathann |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:78
I rebased and squashed the commits which were previously positively reviewed, removing the whitespace changes in |
Changed branch from public/19249 to |
Upstream: http://www.tdi.informatik.uni-frankfurt.de/~lukas/data/tdlib-0.3.1.tar.gz
CC: @dcoudert
Component: packages: experimental
Author: Lukas Larisch
Branch/Commit:
0308199
Reviewer: Nathann Cohen
Issue created by migration from https://trac.sagemath.org/ticket/19249
The text was updated successfully, but these errors were encountered: