Skip to content
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

Add call to cliquer with custom cliques sizes #29269

Closed
Fightlapa mannequin opened this issue Mar 2, 2020 · 68 comments
Closed

Add call to cliquer with custom cliques sizes #29269

Fightlapa mannequin opened this issue Mar 2, 2020 · 68 comments

Comments

@Fightlapa
Copy link
Mannequin

Fightlapa mannequin commented Mar 2, 2020

Currently only maximum/maximal cliques can be obtained by calling cliquer functions. With this change custom cliques can be obtained with cliquer.

Component: graph theory

Keywords: cliquer

Author: Jakub Jabłoński, Jonathan Kliem

Branch/Commit: e1774f7

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/29269

@Fightlapa Fightlapa mannequin added this to the sage-9.2 milestone Mar 2, 2020
@Fightlapa Fightlapa mannequin added c: graph theory labels Mar 2, 2020
@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 2, 2020

Changed author from Fightlapa to Jakub Jabłoński

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 3, 2020

Changed branch from cliquer_custom_size to public/cliquer_custom_size

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 3, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

f085c77Added new function to get induced cliques of custom size in graph

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 3, 2020

Commit: f085c77

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 3, 2020

comment:4

Code could be reused, as this is almost copy-paste of another function. But if it will be fine, will do that and push "clean" version.

@dcoudert
Copy link
Contributor

dcoudert commented Mar 3, 2020

comment:5

Welcome to Sagemath.

You could add tests of corner cases, like small graphs (empty, 1 or 2 vertices), disconnected graphs and graphs with multiple edges.

Have you tested the code on a large graph to see what it supports ?

Also, you will have to improve the coding style (see https://www.python.org/dev/peps/pep-0008/) and ensure that comments are formatted in 80 columns mode.

@dcoudert dcoudert modified the milestones: sage-9.2, sage-9.1 Mar 3, 2020
@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 5, 2020

comment:6

I'm still testing it. I see already one wrong result. I'm investigating it now.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 5, 2020

Changed commit from f085c77 to 78d65d0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 5, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

78d65d0Fixed array size in cliquer. Added doctests

@Fightlapa Fightlapa mannequin added the s: needs review label Mar 5, 2020
@dcoudert
Copy link
Contributor

dcoudert commented Mar 6, 2020

comment:9

a few comments

  • first line of method all_cliques
+    Return the vertex sets of *ALL* the complete subgraphs.
+    Returns the vertex sets of *ALL* the complete subgraphs.
  • add an INPUT block with the description of input parameters
  • if not graph.order(): -> if not graph:
  • I don't understand this case if min_size == 0 and max_size > 0:. Not documented
  • I suggest to free the graph right after the sig_off
  • Could you improve the presentation of the code in cl.c and add some documentation.

I don't know if it is possible, but it would be much better if we could turn this method into an iterator. If the number of cliques to report is large, we may have a segfault. Not clear if the design of cliquer allows that.

@dcoudert
Copy link
Contributor

dcoudert commented Mar 7, 2020

comment:10

According the documentation of cliquer (https://users.aalto.fi/~pat/cliquer.html), it seems possible to avoid storing all cliques (re-entrant methods). So it seems possible to make an iterator, but I don't know how complex it is to do so.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2020

Changed commit from 78d65d0 to f70a90d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

f70a90dAdded input description and made minor refactoring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2020

Changed commit from f70a90d to 60a8166

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

60a8166Added missing characters in input description

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 7, 2020

comment:13

Sorry for those messy commits. Anyway, it's my first time with Cython, I'm not also that familiar with c++. I think I'm not capable of making iterator out of it.

@dcoudert
Copy link
Contributor

dcoudert commented Mar 8, 2020

comment:14
  • The documentation is unclear on whether you search for cliques with number of vertices or edges between min and max, or if you consider the weight of the cliques. Please clarify
  • if the default value of min_size is 0, it must be set if the input, that is def all_cliques(graph, min_size=0, max_size=0):
  • alignement (and text) of input parameters
-    - ``min_size`` -- Search for cliques with weight at least N. If N=0,
-                      searches for maximum weight clique (default).
+    - ``min_size`` -- integer (default: 0); minimum size of the cliques to report.
+      When set to 0 (default), searched for a maximum weight clique.
  • add doctests for the default case of the parameters

  • don't sorted cliques at the end

  • also, could change the branch name to something like public/graphs/29269_all_cliques (i.e., use in a new branch). When the branch is in public/, I can push review commit, which is usually faster for me.

  • concerning the iterator, for the moment you can at least make method all_cliques yield cliques instead of returning a full list. Then we will have to find a way to make method sage_find_all_clique re-entrant, and so avoid to store all cliques.

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 10, 2020

Changed commit from 60a8166 to none

@Fightlapa
Copy link
Mannequin Author

Fightlapa mannequin commented Mar 10, 2020

Changed branch from public/cliquer_custom_size to public/graphs/29269_all_cliques

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

f085c77Added new function to get induced cliques of custom size in graph
78d65d0Fixed array size in cliquer. Added doctests
f70a90dAdded input description and made minor refactoring
60a8166Added missing characters in input description
d39990dChanged all_cliques to iterator

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2020

Commit: d39990d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2020

Changed commit from d39990d to 2859b26

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

a7559betrac #: Merged with 9.1.beta7
2859b26trac #29269: review commit

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:42

a small review commit to avoid using variable size uninitialized.

If OK for you, you can set this ticket to positive review on my behalf. Don't forget to add your name as co-author.

@kliem
Copy link
Contributor

kliem commented Mar 26, 2020

Changed author from Jakub Jabłoński to Jakub Jabłoński, Jonathan Kliem

@vbraun
Copy link
Member

vbraun commented Apr 16, 2020

comment:44

The "Dependencies" field must be a list of trac tickets

@kliem
Copy link
Contributor

kliem commented Apr 17, 2020

Changed dependencies from cliquer to none

@kliem
Copy link
Contributor

kliem commented Apr 17, 2020

comment:45

Sorry, missed that. That's obsolete as cliquer is a standard package.

@vbraun
Copy link
Member

vbraun commented Apr 18, 2020

comment:46

Merge conflict, please fix

@dcoudert
Copy link
Contributor

comment:47

this might be due to #29518. So let's wait for next release to see what's the conflict.

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

Changed branch from public/graphs/29269_all_cliques to public/29269_reb

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

Changed commit from bf4e52d to none

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

comment:49

Replying to @dcoudert:

this might be due to #29518. So let's wait for next release to see what's the conflict.

That's exactly the problem. Doesn't make sense to add a function to a lazy import that will be removed soon. And it should be a method of a graph, this is where it belongs.

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

Commit: e1774f7

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

Changed branch from public/29269_reb to public/29269-reb

@kliem
Copy link
Contributor

kliem commented Apr 25, 2020

New commits:

a3a61cbAdded new function to get induced cliques of custom size in graph
2d5c2e8Fixed array size in cliquer. Added doctests
c0532ceAdded input description and made minor refactoring
f0f1bccAdded missing characters in input description
029814eChanged all_cliques to iterator
7b1e302trac #29269: review commit
ac66919Exposed all_cliques in graphs.py
732fbafmoving deallocations to finally blocks
e1774f7trac #29269: review commit

@dcoudert
Copy link
Contributor

comment:52

LGTM.

@vbraun
Copy link
Member

vbraun commented May 2, 2020

Changed branch from public/29269-reb to e1774f7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants