Skip to content

Conversation

@benabel
Copy link
Contributor

@benabel benabel commented Jan 24, 2021

This implementation use a simple algorithm in which the layout used is
based on a perfect tree of same height in which all leaves would be
regularly spaced.

This approach is ok for little trees to about 4 levels but for larger trees
with holes a better approach would be to implement the Reingold &
Tilford’s algorithm.

@benabel
Copy link
Contributor Author

benabel commented Jan 25, 2021

@joowani Currently the 2.7 and 3.5 tests fails because the code is using f-strings that were added in python 3.5.

https://docs.python.org/3/reference/lexical_analysis.html#f-strings

Do you to maintain compatibility with these python versions even if they have reached their end-of-life:

https://devguide.python.org/devcycle/#end-of-life-branches

@joowani
Copy link
Owner

joowani commented Jan 26, 2021

Hi @benabel,

Thanks for the PR! Yes I am planning to drop support for those two, and I'm also planning to switch over to github actions etc as well. I'll keep this open and re-evaluate after.

Base automatically changed from master to main February 2, 2021 09:37
@joowani
Copy link
Owner

joowani commented Feb 5, 2021

Instead of implementing this method from scratch, I've decided to use python-graphviz instead (it has many more features including Jupyter notebook support) in version 6.0.0. Please see the documentation for more details.

@joowani joowani closed this Feb 5, 2021
@benabel
Copy link
Contributor Author

benabel commented Feb 5, 2021

@joowani using phython-graphviz adds dependecies on python side and on system side with the graphviz binary. For users like teachers that don't have any rights on a machine this feature will be unavailable. You could use a try except and fallback to my implementation if graphviz isn't available on the system.

@joowani joowani reopened this Feb 6, 2021
@joowani joowani changed the base branch from main to dev February 6, 2021 07:00
Copy link
Owner

@joowani joowani left a comment

Choose a reason for hiding this comment

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

Awesome PR!

"""Return the svg representation on current node.
:return: String representation.
:rtype: str | unicode
Copy link
Owner

Choose a reason for hiding this comment

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

This can just be str now.



def _get_coords(values):
"""Generate nodes and edges relative coordinates lists
Copy link
Owner

Choose a reason for hiding this comment

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

Please change to Generate the coordinates used for rendering the nodes and edges.

@@ -0,0 +1,19 @@
from __future__ import absolute_import, unicode_literals
Copy link
Owner

Choose a reason for hiding this comment

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

This can be removed.

""" Module containing layout related algorithms."""


def _get_coords(values):
Copy link
Owner

Choose a reason for hiding this comment

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

Since you are importing this outside of the module, should be renamed to get_coords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only used on layout module and test.

@joowani
Copy link
Owner

joowani commented Feb 6, 2021

Hi @benabel,

I like your idea. I've reopened the PR and left some comments. Could you please address them and rebase? Thanks again for your contribution.

@joowani joowani force-pushed the dev branch 2 times, most recently from 1a32744 to a2fd4ec Compare February 10, 2021 10:15
@benabel
Copy link
Contributor Author

benabel commented Feb 11, 2021

@joowani I tried to amend the code using your suggestions, however I can't pass the doctests.

@benabel
Copy link
Contributor Author

benabel commented Feb 12, 2021

@joowani In fact the doctests fails due to the graphviz method. My changes seems ok, I added typings and unit tests passes.

graphviz doesn't seem to be installed in build.yaml.

2 Errors in task Run Sphinx doctest:

Failed example:
    graph.body              # Get the DOT body
Expected nothing
Got:
    ['\t140536390881360 [label="<l>|<v> 4|<r>"]', '\t140536390881360:l -> 140536390659728:v', '\t140536390881360:r -> 140536390660048:v', '\t140536390659728 [label="<l>|<v> 13|<r>"]', '\t140536390659728:l -> 140536390659472:v', '\t140536390659728:r -> 140536390657424:v', '\t140536390660048 [label="<l>|<v> 1|<r>"]', '\t140536390660048:l -> 140536390573584:...
graphviz.backend.ExecutableNotFound: failed to execute ['dot', '-Kdot', '-Tpdf', '-O', 'Digraph.gv'], make sure the Graphviz executables are on your systems' PATH

@joowani joowani merged commit d81833a into joowani:dev Feb 16, 2021
@joowani
Copy link
Owner

joowani commented Feb 16, 2021

Hi @benabel,

No worries, I can take care of the clean up. Thanks again for your contribution! This is very useful.

@benabel
Copy link
Contributor Author

benabel commented Feb 16, 2021

You're welcome @joowani. Thanks for sharing this package.

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