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

Implementation of algorithm for exact counting graph homomorphisms #38062

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

guojing0
Copy link
Contributor

@guojing0 guojing0 commented May 23, 2024

In the past few months, we have been working towards a SageMath library for exact counting graph homomorphisms, based on the proof of Prop. 1.6 from "Homomorphisms Are a Good Basis for Counting Small Subgraphs" by Radu Curticapean, Holger Dell, and Dániel Marx: The algorithm is deterministic and runs in time $\exp(O(k)) + poly(k) \cdot n^{tw(H) + 1}$ for counting the number of homomorphisms from graph $H$ to graph $G$, where $k = |V(H)|$, $n = |V(G)|$, and $tw(H)$ is the treewidth of $H$.

The library is recently released on GitHub: https://github.com/guojing0/count-graph-homs

This commit is mainly for the sake of CI. More documentations and tests will follow.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • 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

@guojing0 guojing0 self-assigned this May 23, 2024
@guojing0 guojing0 added this to the sage-10.4 milestone May 23, 2024
@guojing0 guojing0 requested a review from dcoudert May 23, 2024 10:24
@guojing0 guojing0 changed the title Implementation of exact counting graph homomorphisms functionality Implementation of algorithm for exact counting graph homomorphisms May 23, 2024
Copy link

github-actions bot commented May 23, 2024

Documentation preview for this PR (built with commit ffd345d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR requires some work before. It must be properly documented, doctests are needed, etc. Here is a first list of needed changes.

I don't think we need this helper_functions.py file. I fact, I'm in favor of using a single file called homomorphisms.py and gathering all methods related to graph homomorphisms.

src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved
src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved
"""
# Whether it's BFS or DFS, every node below join node(s) would be
# computed already, so we can go bottom-up safely.
for node in reversed(self.dir_labelled_TD.vertices()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify the order in which you want to visit the vertices. Here, you take the reverse order of the list returned by vertices() but vertices() does not ensure any kind of ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read the implementation code for vertices(), since self.dir_labelled_TD is a DAG, I assume in this case, the vertices returned would/should be from the source to the sink(s)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we cannot make any assumption on this ordering. It depends on internal data structures and not on the orientation of the graph. If you need a specific ordering, you must use an appropriate graph traversal method (BFS, DFS, etc.).

src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved

def extract_bag_vertex(mapping, index, graph_size):
r"""
Extract the bag vertex at `index` from `mapping`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method don't extract but returns an index


def add_vertex_into_mapping(new_vertex, mapping, index, graph_size):
r"""
Insert `new_vertex` at `index` into `mapping`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method don't insert but returns the index where to do so.

src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved
src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved
@guojing0
Copy link
Contributor Author

guojing0 commented Jun 5, 2024

This PR requires some work before. It must be properly documented, doctests are needed, etc. Here is a first list of needed changes.

Thank you for all the reviews! Of course, many tests and much documentation are needed.

I don't think we need this helper_functions.py file. I fact, I'm in favor of using a single file called homomorphisms.py and gathering all methods related to graph homomorphisms.

I also consider the possibility of having only one file, but wouldn't it make the main file count_homomorphisms.py too long? It's already 230+ lines.

I think the name count_homomorphisms.py would better reflect what the goal(s) this file intends to achieve, i.e., counting homomorphisms; homomorphisms.py sounds somewhat general.

@dcoudert
Copy link
Contributor

dcoudert commented Jun 5, 2024

I also consider the possibility of having only one file, but wouldn't it make the main file count_homomorphisms.py too long? It's already 230+ lines.

We already have files with 20.000+ lines.


if target_density >= self.density_threshold:
target = self.actual_target_graph.adjacency_matrix(
vertices=self.actual_target_graph.vertices()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best if you fix the order in which you want to consider the vertices, assign it to a list and use it. Otherwise you are not sure what may happen.

src/sage/graphs/homomorphisms/count_homomorphisms.py Outdated Show resolved Hide resolved
"""
# Whether it's BFS or DFS, every node below join node(s) would be
# computed already, so we can go bottom-up safely.
for node in reversed(list(self.dir_labelled_TD.breadth_first_search(min(self.dir_labelled_TD)))):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that min(self.dir_labelled_TD) is the root ? isn't it safer to search for a vertex with in-degree 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that min(self.dir_labelled_TD) is the root ?

By construction, yes.

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

Successfully merging this pull request may close these issues.

2 participants