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

is_comparability() fails for immutable graph #25550

Closed
jm58660 mannequin opened this issue Jun 10, 2018 · 19 comments
Closed

is_comparability() fails for immutable graph #25550

jm58660 mannequin opened this issue Jun 10, 2018 · 19 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jun 10, 2018

g = DiGraph({0: [1]}, immutable=True)
sage.graphs.comparability.is_comparability(g)

outputs IndexError: list index out of range. Works with immutable=False.

This follows from this:

g = DiGraph({0: [1]}, immutable=True)
print g.neighbors(1)

g = DiGraph({0: [1]}, immutable=False)
print g.neighbors(1)

which outputs 0 and 1.

Component: graph theory

Author: David Coudert

Branch/Commit: 8080e05

Reviewer: Travis Scrimshaw

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

@jm58660 jm58660 mannequin added this to the sage-8.3 milestone Jun 10, 2018
@jm58660 jm58660 mannequin added c: graph theory labels Jun 10, 2018
@jm58660

This comment has been minimized.

@dcoudert
Copy link
Contributor

Author: David Coudert

@dcoudert
Copy link
Contributor

Commit: 43dff26

@dcoudert
Copy link
Contributor

Branch: public/25550_neighbors_nbrs

@dcoudert
Copy link
Contributor

comment:2

Right, method iterator_nbrs in static_sparse_backend.pyx only considered out neighbors. This should fix it.


New commits:

43dff26trac #25550: fix iterator_nbrs

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2018

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

fc1b7b0trac #25550: Merged with 8.3.rc3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2018

Changed commit from 43dff26 to fc1b7b0

@dcoudert dcoudert modified the milestones: sage-8.3, sage-8.4 Jul 29, 2018
@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

comment:5

LGTM modulo two trivial things:

  • Could you add a test showing the need for the seen set (to help prevent regressions)?
  • Why the print in this test: print(g.neighbors_in(1))?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

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

508cf3etrac #25550: deal with multiedges in neighbor iterators

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

Changed commit from fc1b7b0 to 508cf3e

@dcoudert
Copy link
Contributor

comment:7

I have changed the seen data structure. I'm not sure it's best way to do it. May be a set is easier and sufficiently fast here.

One issue I don't know how to solve is: were to free the bitset ? I don't find appropriate dealloc method. Should I add it ?

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

comment:8

I don't like how you are using _seen as it gets doubly used: create one iterator, advance it 1 step, then create another iterator and _seen becomes empty. I think you are better defining it local to each function where you free it upon exit.

I also would like to see a test for 0 <-> 1 (an arrow point each way) so that 1 does not appear twice as a neighbor of 0.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

Changed commit from 508cf3e to 8080e05

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2018

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

8080e05trac #25550: local use of seen

@dcoudert
Copy link
Contributor

comment:10

You are perfectly right. Should be better now. I'm using set again since it might be sufficiently fast here.

@tscrim
Copy link
Collaborator

tscrim commented Jul 30, 2018

comment:11

Thanks. LGTM.

@dcoudert
Copy link
Contributor

comment:12

Thank you.

@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

Changed branch from public/25550_neighbors_nbrs to 8080e05

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