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

bennaaym/graphviz_support: bug fixed | README updated with examples #57

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bennaaym
Copy link
Contributor

@bennaaym bennaaym commented Feb 8, 2022

Description

This is an update of the LabgraphGraphviz extension. The PR adds the following :

  • Fix a small bug that appears sometimes when the graph contains a group( the bug was mainly caused by the wrong usage of Python Set.pop() method, which may give different output for the same input)
  • Add examples on how to use the generate_graphviz function
  • Fix package issues (import graphviz_support module)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

validation/testing

  • Tested on graph examples that include a group unit

Checklist:

  • Have you added tests that prove your fix is effective or that this feature works?
  • New and existing unit tests pass locally with these changes?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 8, 2022
@bennaaym bennaaym force-pushed the bennaaym/graphviz_support branch 2 times, most recently from b3b3142 to 1f312b1 Compare February 8, 2022 20:32
@fzchriha
Copy link
Contributor

Hey @bennaaym great job on these examples, and thank you so much for adding them. I just wanted to point out a small bug, I encountered when trying to run generate_graphviz.py, see line 174

Screen_Shot_2022-02-10_at_3 31 02_PM

When I tried debugging it, it turned out my output_file was /Users/fatimazahra.chriha/Desktop/mlh/spring2022/labgraph/extensions/graphviz_support/graphviz_support/examples/output/simple_viz.svg

So when running filename, format = output_file.split('.') it resulted in a list of 3 elements instead of 2 I solved it by replacing line 174 by

split_list = output_file.split('/')
last_ele = split_list.pop().split('.')
format = last_ele[1]
filename = '/'.join(split_list) + '/' + last_ele[0] 

Just wanted to give you a heads up, awesome work!

@bennaaym
Copy link
Contributor Author

bennaaym commented Feb 10, 2022

Hello, @fzchriha thank you for your feedback. I have noticed that bug and I fixed it (please check the PR update). I like your solution, however, I think using "os.path.splitext()" will be an easier option. Please try it again and let me know if there are any other issues

@jfResearchEng jfResearchEng mentioned this pull request Apr 20, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants