Wrong result with tdlib for treewidth #38159
Milestone
Comments
5 tasks
|
Another example with only 10 nodes sage: G = Graph('I~~}vPlr_')
sage: G.treewidth(algorithm='sage')
5
sage: G.treewidth(algorithm='tdlib')
6 |
|
I'm not familiar with the compressed string representation. Please remind me how to convert Which version of tdlib (aka treedec) is this, and which algorithm does it use? Thanks |
|
See also: |
|
I figured that It looks like this in gr format With this input, Similar decompositions also from fi, ppfi and bmd (all with tw 5, if tw is the size of a biggest bag minus 1). I can't say much about the cython sage integration, and obviously the results may depend on vertex order and such things. Do you actually get a suboptimal decomposition? Which one? |
|
The graph has 10 vertices and 33 edges and we get the decomposition: We clearly have to use a newer version of tdlib. We currently call a former function |
|
On Thu, Jun 06, 2024 at 11:33:16PM -0700, David Coudert wrote:
The graph has 10 vertices and 33 edges
Thanks, interesting. I used a tool "showg" to decode the string. Then
hand edited the output... It had to go wrong.
The current algorithms also work on the 33 edge graph, but I havent
tried the dated exact decomposition (cutset I suppose), as it's not
enabled in tdecomp.
We clearly have to use a newer version of tdlib. We currently call a
former function `sage_exact_decomposition` from version `0.3.1.p0`. We
will have to figure out how to use the current interfaces.
The new python bindings need work. Perhaps it should be split out, and
become more "pythonic", with a dependency on libtreedec provided by the
main package. Time will tell, suggestions welcome.
|
The best would be to have a Python package on PyPI, dependent on libtreedec. Then it would become the only point of entry to tdlib in Sage, simplifying things. |
|
On Fri, Jun 07, 2024 at 05:09:40AM -0700, Dima Pasechnik wrote:
The best would be to have a Python package on PyPI, dependent on
libtreedec. Then it would become the only point of entry to tdlib in
Sage, simplifying things.
If I understand correctly, we should eventually build a shared library.
A starting point could be the various treedec/_*.cpp minus the
BOOST_PYTHON_MODULE wrapper code.
Apparently, these steps are needed.
- copy treedec to lib
- remove all references to python in lib/*
- adjust rules in lib/*, link it all into libtreedec.so
- do the opposite in treedec/*, link with -ltreedec
- (examples/treedec: also use libtreedec)
- .. whatever else to meet PyPI needs.
All we need is a volunteer, but count me in as a reviewer.
|
|
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 10, 2024
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> - FIxes sagemath#30813 - Fixes sagemath#38159 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> - Depends on sagemath#38144 (merged here for testing) URL: sagemath#38163 Reported by: Matthias Köppe Reviewer(s): David Coudert
The question is whether to use Cython, or instead use Boost/Python or pybind11. The latter is a Sage package, and is used in scipy, contourpy, etc. It's said to be lighter and easier that Boost/Python. |
one can also can take Cython/C++ code in Sage and modify it appropriately. |
Have you tried running In fact, I'm missing a proper documentation on how to install each of our optional packages. I have the filling that we have many ways and that we should sometimes favor one of them, but not always the sage depending on the package... |
|
Also, a clean install errors out with the same error. PS. the error is actually not so obscure - there is a lookup happening for wheels for |
|
One way or the other, #38163 should add the test from the example here. |
|
I agree (#38163 (comment)) |
2 tasks
|
Just open a PR for that please. |
dimpase
added a commit
to dimpase/sage
that referenced
this issue
Jun 13, 2024
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 16, 2024
This will fix sagemath#38159 Add a missing test ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] The description explains in detail what this PR is about. - [ x] I have linked a relevant issue or discussion. - [ x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> sagemath#38163 URL: sagemath#38214 Reported by: Dima Pasechnik Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 16, 2024
This will fix sagemath#38159 Add a missing test ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] The description explains in detail what this PR is about. - [ x] I have linked a relevant issue or discussion. - [ x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> sagemath#38163 URL: sagemath#38214 Reported by: Dima Pasechnik Reviewer(s): David Coudert
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 17, 2024
This will fix sagemath#38159 Add a missing test ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ x] The title is concise and informative. - [ x] The description explains in detail what this PR is about. - [ x] I have linked a relevant issue or discussion. - [ x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> sagemath#38163 URL: sagemath#38214 Reported by: Dima Pasechnik Reviewer(s): David Coudert
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Steps To Reproduce
Expected Behavior
Actual Behavior
Additional Information
No response
Environment
Checklist
The text was updated successfully, but these errors were encountered: