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 subgraph_hash in dot parser. #129

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arbipher
Copy link

@arbipher arbipher commented Nov 26, 2022

This PR adds a new entry function parse_all to get subgraph information when parsing a dot file.

In the old code, clust_nodes is defined, and used, but not returned.
It's replaced by a graph_hash which records the nodes, attr, and parent using the parsing.

The previous parsing functions are only changed to drop not-use part of create_graph_and_clusters''s return values.

I make an issue #130 for more discussion.

@arbipher arbipher changed the title Add subgraph hashi n dot parser. Add subgraph_hash in dot parser. Nov 26, 2022
@backtracking
Copy link
Owner

Many thanks for this PR and #130. I'll have a look at it and I'll merge the two of them if possible.

A quick question: have you checked that any .dot files in the repo (notably in editor/tests/ and dgraph/examples/) is still parsed successfully, in a backward compatible way?

@arbipher
Copy link
Author

Not yet. I forgot there are dot files inside these two folders.

I can add a stanza in tests/dune to ensure all the dot files can parse successfully.

My PR currently is not so good, not only by (#130 Q2 Q3) but also my added structure for subgraph graph overlaps with the existing clust_attr. In design, graph contains the information of

  1. subgraph attr (recorded in clust_attr in old code, but not complete due to lack of handling Eq (id, id))
  2. subgraph nodes (recorded in clust_node in old code, but not returned)
  3. subgraph parenting relation (not recorded in the old code)
type graph = {
  sg_nodes : string list;
  sg_attr : attr list;
  sg_parent : string option;
}

I am also hoping I can remove clust_attr during the parsing but derive it in the end to satisfy the compatible function parse_bounding_box_and_clusters

@arbipher
Copy link
Author

arbipher commented Nov 1, 2023

A quick question: have you checked that any .dot files in the repo (notably in editor/tests/ and dgraph/examples/) is still parsed successfully, in a backward compatible way?

It seems to take me nearly one year to answer this quick question 🎃.

I am working on a WIP PR to test parsing those dot files with the current dot parser and my modified parser, then check the equality of the results.

I added a dune alias dot_parse in tests/dune and tests/dot_parse.ml.
Just dune build @runtest3

I copied the current src/dot.{ml,mli} to src/dot_v1.{ml,mli} and made my modified parser at src/dot.{ml,mli}.
I have to add one line at module Dot_V1 = Dot_V1 to export Dot_V1.
Then in test/dot_parse.ml, I compare the parsing results from both Dot and Dot_V1.

However, I don't figure out a good idea (ocamlgraph-way) to check the exactly equality of these two graphs in test/dot_parse.ml.

Now I just check they share the same number of vertices and edges. (The specific problem is G.mem_vertex g2 v1 always returns false)

I think some files just for testing during the PR discussion. When the issue is settled down, I can remove them.

@arbipher arbipher reopened this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants