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
Poset.dimension #17540
Comments
Branch: u/ncohen/17540 |
Commit: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:4
Is the ticket description wrong? I can't review code that is not supposed to work :-) |
comment:5
Don't get it. You talk about the missing () to dimension ? |
comment:6
Replying to @nathanncohen:
we are not able to |
This comment has been minimized.
This comment has been minimized.
comment:7
I am glad to see this very confusing typo corrected. Thank you for bringing it to my attention. |
This comment has been minimized.
This comment has been minimized.
comment:8
IMHO there should be a separate function to compute the chromatic number of a hypergraph. |
comment:9
It is true, but we could not call it from here. The reason why this code is so long is that we avoid building the LP from the start every time we need to add a constraint. It is only re-built when we notice that the new hypergraph is not k-colorable anymore. Nathann P.S. : Actually I will write a LP for that. I will surely need it some day. |
comment:10
Done at #17542. Nathann |
comment:11
Replying to @nathanncohen:
Hmm, why do you need to rebuild the hypergraph? The latter is fixed. What is not fixed is the number of colours you need, and you don't want to rebuild the ILP from scratch for this. Why is this poset-specific? |
comment:12
On the paper, the hypergraph is very well defined and you have to compute its chromatic number. In practice, you cannot build the hypergraph because the list of its edges is too huge (I would not know how to do that anyway). So what we do is start with some of the edges in the hypergraph, and find a coloring. When we have a coloring, we can check that it is valid by building the corresponding graphs --> if all are acyclic that's fine and we are done. If one is not acyclic, however, we have found a cycle --> a new set to add to our hypergraph. And so we re-color this thing again. This being said, when you find a new set in your hypergraph, you only have to add one constraint and then you can call Nathann |
comment:13
according to [FT00], instead of the hypergraph on |
comment:14
Concerning the Also, the "just to be sure" section at the end of the code should be run only if some By the way, in the |
comment:15
I do not see how that would change anything. You are just telling me that only a subset of points of the hypergraph matters, but that does not give me the set of edges that only involve critical pairs. Nathann |
comment:16
No it cannot. The problem is NP, so there is a certificate for a 'yes' answer, but it is not co-NP so there is no certificate for the other side. (Insert 'polynomial' wherever it fits in the sentence, and assume P!=NP)
I do not think that it should, considering the running time of the algorithm. That's almost for free comparatively, so I don't see the need to make it optional.
Well I usually write it the way it is written. If you want to change it you are welcome to add a commit, but if it is just about enforcing the coding style you like I will not code it for you Nathann |
comment:17
Replying to @nathanncohen:
This is not the coding style i like, it is about coding conventions, a feature that allow various people to collectively work on some source code, see http://sagemath.org/doc/developer/coding_basics.html#documentation-strings |
comment:18
There seems to be no check for argument, i.e. One-line explanation on TOC should end with dot. Does this work with empty poset? Checking just now, but compiling is slooow with this machine. |
comment:19
Replying to @nathanncohen:
It seems that you get a smaller ILP; you run your acyclicity test and compute cycles just as you do, but only leave critical pairs there (i.e. you take the subhypergraph induced on the critical pairs --- the edges get intersected with the subset of critical pairs). |
comment:20
If the fact that I wrote the defaut value at the end of the sentence instead of the beginning is a problem for you, add a commit. You will find many other such instances in the same document. Nathann |
comment:21
I do not think that there should, especially when the argument is boolean (not even a string) and when it does not produce any bug whatever happens. Chekking this kind of stuff is useful when a string is expected, but really if we begin to check every single arguments of every function like that, then half of the python code we have in Sage will be checking the type of input because Python does not do it instead.
Done
Done (Jori: we have a 4h30 difference in our timezones) Nathann |
comment:23
I see. Well, I am not very eager to dig into that in order to see how I could list all those critical pairs, then add this to this already non-trivial code, given that I am not very convinced that it will make any difference in the runtimes Nathann |
comment:24
Ping ?... |
comment:25
Piiiiing ?... |
comment:26
Piiiiiiiiiiiiiiiiiing ?... |
comment:27
OK, OK, just a bit of patience... |
comment:28
what is your default LP solver (on which that was tested)?
runs, and runs, and runs (that's with GLPK). |
comment:29
sigh... That's CPLEX |
comment:30
With GLPK for
With CPLEX it takes 252ms. And for Nathann |
comment:31
for G=K33 and with GLPK your code seems to be looping forever. In the code you call PS. OK, I see, you increase k every time it goes up. Anyhow, one gets stuck on some LP at k=3 here... |
comment:32
Indeed.
Well, it depends on the graph. And of course it depends on the solver |
comment:33
OK, tried this with GUROBI, it did work for K33. Trying for Petersen graph now :-) typo:
do you mean |
comment:34
Yes. Tell me when you are done with the review and I will fix everything at once. |
comment:35
I also notice that there is no objective function in LP. Sometimes specifying some more or less random non-symmetric function helps to speed things up. |
comment:36
OK, fix that typo, and I'll set it to positive review, even though the code is very slow... |
comment:37
The Wikipedia article you refer says that this is sometimes also called "Dushnik–Miller dimension". Is this common name for dimension? If so, it should be mentioned somewhere, so that user googling "Dushnik–Miller sagemath" will found this. |
comment:39
Here it is! Nathann |
Reviewer: Dima Pasechnik |
comment:41
Thaaaaaaaaaaaaaanks ! Nathann |
Changed branch from u/ncohen/17540 to |
With this code, Sage at last can compute the dimension of a Poset.
http://en.wikipedia.org/wiki/Order_dimension
Nathann
See https://groups.google.com/d/topic/sage-devel/qEE9YUAPA4g/discussion
CC: @miguelmarco @dimpase @videlec @sagetrac-tmonteil @jm58660
Component: combinatorics
Author: Nathann Cohen
Branch/Commit:
ffe181b
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/17540
The text was updated successfully, but these errors were encountered: