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

Error in is_clique for graphs with multiple edges #25696

Closed
dcoudert opened this issue Jun 28, 2018 · 25 comments
Closed

Error in is_clique for graphs with multiple edges #25696

dcoudert opened this issue Jun 28, 2018 · 25 comments

Comments

@dcoudert
Copy link
Contributor

The following graph is not a clique

sage: G = Graph(multiedges=True)
sage: G.add_cycle([0, 1, 2, 3])
sage: G.add_edge(0,1)
sage: G.add_edge(0,1)
sage: G.is_clique()
True

CC: @pelegm @jm58660 @fchapoton

Component: graph theory

Keywords: days94

Author: David Coudert

Branch/Commit: 50df10e

Reviewer: Jori Mäntysalo

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

@dcoudert dcoudert added this to the sage-8.3 milestone Jun 28, 2018
@dcoudert
Copy link
Contributor Author

comment:1

There might be smarter way to do that, but at least it fix the issue


New commits:

80bbcdatrac #25696: fix issue with is_clique

@dcoudert
Copy link
Contributor Author

Commit: 80bbcda

@dcoudert
Copy link
Contributor Author

Branch: u/dcoudert/25696_is_clique

@pelegm
Copy link
Contributor

pelegm commented Jun 30, 2018

comment:3

I'm not sure we have the right/common definition of cliques here. Docs say:

A clique is a set of vertices such that there is an edge between any two vertices.

I would say:

A clique is a set of vertices such that there is a single edge between any two vertices.

For example, according to this paper here,

A clique in a graph or multigraph G is a simple complete subgraph of G.

@pelegm
Copy link
Contributor

pelegm commented Jun 30, 2018

Changed keywords from none to days94

@dcoudert
Copy link
Contributor Author

comment:5

What we expect certainly depends on the context. One solution could be to add a parameter strict, such that when strict==True we want a simple complete subgraph, and when set to False, we may tolerate loops of multiple edges = contains a simple complete subgraph. This is certainly not perfect, I agree.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2018

Changed commit from 80bbcda to 34d6cea

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2018

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

160e6b9trac #25696: Merged with 8.3.beta8
34d6ceatrac #25696: add parameters induced and loops

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2018

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

de4e89ctrac #25696: fix failing doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2018

Changed commit from 34d6cea to de4e89c

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 7, 2018

comment:8

I added parameter induced to check for an induced clique or if the graph contains a clique. I also added a parameter loops to deal with complete (di)graphs with loops. This way we have more flexibility, and we can get what we want.

The last commit exhibit a potential change of behavior when asking for an undirected clique inside a directed graph. I consider that we can have only 1 arc between a given pair of nodes to get an induced clique. Let me know if you agree.

@dcoudert

This comment has been minimized.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 11, 2018

comment:10

The logic seems to be clear. However,

sage: g = Graph({1: [2]})
sage: g.is_clique([1,2,3])
True

Also the indentation in INPUT section for parameters vertices and directed_clique is wrong (but right for induced and loops).

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 11, 2018

comment:11

Also

sage: g = Graph(3)
sage: g.is_clique([1,2,3])
False
sage: g = Graph(2)
sage: g.is_clique([1,2])
True

@jm58660 jm58660 mannequin added s: needs work and removed s: needs review labels Jul 11, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

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

76fc46btrac #25696: Merged with 8.3.rc0
50df10etrac #25696: fix reviewers comments

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

Changed commit from de4e89c to 50df10e

@dcoudert
Copy link
Contributor Author

comment:13

Good catch ! This is a side effect of g.subgraph(vertices) that don't check if vertices is a subset of the vertices of the graph. I added a simple test.

Do you think I should open a ticket to raise an error in g.subgraph(vertices) when some vertices (or edges) are not in the graph ?

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 11, 2018

comment:14

Replying to @dcoudert:

Do you think I should open a ticket to raise an error in g.subgraph(vertices) when some vertices (or edges) are not in the graph ?

To me it sounds logical. I expect that

SymmetricGroup(2).subgroup((1,2), 'junk')
Poset({1: [2]}).subposet([1, 'junk'])
Graph({1:[2]}).subgraph([1, 'junk'])

gives similar output (they do not), and actually I have changed the output of

Poset({1: [2], 3: []}).is_antichain_of_poset([1,2,4])

to be an error message, not False.

However, if you do that, then this ticket should be postponed until that is ready.

@dcoudert
Copy link
Contributor Author

comment:15

I prefer to have this ticket fixed now since it impacts other methods. For instance, g.vertex_connectivity() has special treatment for cliques. If g.is_clique() returns a wrong answer, then the answer of g.vertex_connectivity() is also wrong.

We can deal with the subgraph problem independently.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 11, 2018

comment:16

Replying to @dcoudert:

I prefer to have this ticket fixed now since it impacts other methods. For instance, g.vertex_connectivity() has special treatment for cliques. If g.is_clique() returns a wrong answer, then the answer of g.vertex_connectivity() is also wrong.

We can deal with the subgraph problem independently.

OK. I'll try to review this in a day or two.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 12, 2018

Reviewer: Jori Mäntysalo

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 12, 2018

comment:17

Seems to be OK. ("Trivially complicated code", but there is no other way as there are so many combinations of parameters.)

@dcoudert
Copy link
Contributor Author

comment:18

Thank you.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 12, 2018

comment:19

rc for 8.3 is already out.

@jm58660 jm58660 mannequin modified the milestones: sage-8.3, sage-8.4 Jul 12, 2018
@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

Changed branch from u/dcoudert/25696_is_clique to 50df10e

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