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

Fix Diagram.make_dot for pydot 3.* compatibility #1177

Merged
merged 8 commits into from
Sep 9, 2024

Conversation

ethho
Copy link
Contributor

@ethho ethho commented Sep 6, 2024

Encapsulates node names and edge attribute attr_map in double quotes before calling to_pydot in method Diagram.make_dot. Partially duplicates changes made in #1176.

Closes #1175 and closes #1169 and closes #1100 and closes #1065. May close related issue #1072.

Before running `to_pydot`, we encapsulate node names
and attr_map in double quotes to avoid syntax errors
when the node names contain special characters.
Implements the workarounds described in
#1176 (comment)
and
pydot/pydot#258 (comment)
@ethho ethho self-assigned this Sep 6, 2024
Copy link
Contributor Author

@ethho ethho left a comment

Choose a reason for hiding this comment

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

@tabedzki Could you review? If approved, we can merge into your PR to keep the issue board clean.

datajoint/diagram.py Show resolved Hide resolved
datajoint/diagram.py Show resolved Hide resolved
@tabedzki
Copy link
Contributor

tabedzki commented Sep 6, 2024

@ethho This looks good from first glance. I'll try to check this sometime over the weekend/monday to confirm that this works and get back to you on this. Thank you for addressing this so quickly.

@ethho
Copy link
Contributor Author

ethho commented Sep 6, 2024

Sure thing, thank you @tabedzki for doing the heavy lifting of investigating!

@tabedzki
Copy link
Contributor

tabedzki commented Sep 7, 2024

@ethho re-running the same example that I used for #1176, this works without requiring the latest version of networkx.
I wouldn't merge this into my fix as there as my fix requires the latest version whereas this works with current pip versions of both packages and merging it in introduces some key errors due to the extra " in the names. This alternative sidesteps that issue. Your changes also have the added benefits of not needing to update the tests and supporting older versions of Python.

I'd say all that is left is to update the change log and list out all the issues here that this PR closes so clear up some of the backlog. I'll close my PR.

Package versions:
networkx 3.3
pydot 3.0.1

DOT representation of the graph truncated:

strict digraph  {
"ephys_element.Clustering" [primary_key="{'insertion_number', 'precluster_param_steps_id', 'paramset_idx', 'recording_id'}", distinguished=False, node_type=<class 'datajoint.user_tables.Imported'>, shape=ellipse, fontsize=12, fontcolor="#00007FA0", fontname=arial, fixedsize=false, width=0.48, height=0.48, tooltip="&#8594; ephys_element.ClusteringTask&#13;------------------------------&#13;clustering_time      &#13;package_version=\"\"   &#13;", label="ephys_element.Clustering", color="#00007F40", style=filled];
}
``

@ethho ethho marked this pull request as ready for review September 8, 2024 18:26
@ethho ethho changed the title Fix Diagram.make_dot Fix Diagram.make_dot for pydot 3.* compatibility Sep 8, 2024
@dimitri-yatsenko dimitri-yatsenko merged commit 10ca20b into master Sep 9, 2024
21 checks passed
@dimitri-yatsenko dimitri-yatsenko deleted the fix/diagram-make-dot branch September 9, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment