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 hierarchical layout #4468

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

Conversation

rocky
Copy link
Contributor

@rocky rocky commented Dec 17, 2020

No description provided.

@rossbar
Copy link
Contributor

rossbar commented Dec 20, 2020

This seems to be a proposal for adding a new graph layout algorithm. Would you mind providing a few more details on the layout and use-cases? I'm not familiar with it and took a quick glance at the linked paper in the reference but didn't catch anything specific about this layout/algorithm. Thanks!

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I haven't looked at this in detail, but a few things jumped out:

  1. NetworkX follows the numpydoc docstring standard
  2. I'm -1 on having the two functions, espeically since hierarchy_layout only exists as a pass-through function to run hierarchy_layout_with_min_sep but return only the pos. IMO it would be better to rename hierarchy_layout_with_min_sep -> hierarchy_layout and not return the min_sep value at all so that this conforms with all the other layout algorithms.

@jarrodmillman
Copy link
Member

Would you mind adding an example to the gallery in the "Drawing" section? Just create a new file examples/drawing/plot_<something>.py. You can look at the other examples to see you to do it, but basically all you need to do is create a docstring at the top of the file with the title of the plot for the gallery and then write a Python script that draws a tree using the hierarchical layout.

I recently used multipartite_layout to draw a circuit:
https://github.com/networkx/networkx/blob/master/examples/algorithms/plot_circuits.py

At the time, I wanted a more tree-like layout with the root at the top. Would you mind converting that example to use the hierarchical layout as well?

@rocky
Copy link
Contributor Author

rocky commented Dec 22, 2020

@jarrodmillman

Over the holidays, I will work up some networkx examples to show this off. (But see below...)

I had also been wondering about whether the readthedoc information on layouts needs to be adjusted.

As for your circuit layout, I am not sure this can be used just yet — the code right now makes a check to see that the input is a tree and barfs if it is not. This "design" comes from the code cited in the text.

I had been considering expanding that to allow DAGs by starting with a spanning tree. However in my mind that is something for later. When contributing to open-source projects, I like to start out small and do more and more ambitious things after gauging how difficult it is to get previous PRs into the code base.

So to address some of @rossbar 's comments let me elaborate a little bit on the use of this and my workflow and the other issues and questions raised.

I am currently using networkx in Mathics to provide graphs. The specific plugin code I am using hasn't been released on PyPI yet though.

However in the screenshots directory you will find some examples of the use of this for Binomial trees of depth 3 and 6, and the same thing for Complete K-Ary trees of depth 3 and 6.

The exact sequence of commands that I ran can be found in console.md.

If you look at the files with uncorrected in the name such as CompleteKaryTree-7-uncorrected.png you will see why the "min-sep" is helpful in the layout — it determines the edge and node sizes. (Well the number of nodes in the graph and the fact that it is a tree also helps too). Without some sort of correction based on the spacing the lines and node shapes can mush together.

If there is a more networkx-consistent way to pass back this layout-specific information so that it can be used to the plotting routine (here matplotlib), that would be great. For now I am using min-sep in my tree layout routine.

I asked about a routine to look at a graph and adjust things based found in the layout on the networkx-discuss group but didn't receive a response. So basically right now I have rolled my own routine.

Finally let me address the style issues and PRs in general. It is not uncommon when someone doesn't have anything to say about the code to make a comment on the style :-) I am used to this.

Each project has its own style and the great thing about standards is that there are so many of them. Perhaps one day "black" will do this and you'll have a github hook which automatically corrects this as opposed to throwing this kind of stuff back on the programmer's lap.

Now don't get me wrong here, I understand the usefullness of havng a consistent style. It makes sense for each project to throw its weight behind which one it feels is true one right way.

For me though I'd rather be spending the time addressing the paucity of layouts. One of them mentioned in Steve Skiena's Combinatorica books is a ranked layout. I am not sure if that is the same thing as a "multipartite" layout.

A challenge I have found in my open-source projects is getting people to contribute. This very definitely happened in the Mathics project where various simple requests were made of PRs, there was no response and so these PR's have lingered years before I and others got involved and made whever corrections were deemed necessary.

It should be no surprose then that more difficult it is to get changes in the few PR's you are going to get. In my case, it is just a matter of having the time to do this. Perhaps you have tools that do this? I don't. If there is say a GNU Emacs package to just do this it to make this easier, let me know.

That said, sure, I'll go change the code to be numpydoc compliant. However this bring me to the other thing that personally I find weird about PRs. (I am not necessarily saying though this applies here.)

At least for me and my open-source projects I try to view PR's as a more collaborative effort. So please if you are disposed to add more commits to this to correct the kinds of things that you find offesive, please do so! I know some programmers are very fussy about having someone else change or correct their code. I take no such offense.

That said, probably over the upcoming holiday I will make a pass at changing the style. Sure, if you want to remove min-sep because right now networkx doesn't have a framework for doing such things, fine. I'll just keep using my modified version.

But I should also say that over the holiday I also plan on putting a release out for Mathics 1.1.1 (which will include finally releasing that graph code to PyPI) so that has priority for the limited time I can spend on this.

* Use numpy-style docstring for describing parameters and giving
  examples
* Add two examples/drawing/plot_*.py demo programs
* Add to automatic imports
@rocky
Copy link
Contributor Author

rocky commented Dec 29, 2020

@rossbar @jarrodmillman Most of the changes you suggested have now been made.

I've left in minimum separation return value in, but if you feel strongly about that feel free to remove that function and adjust the examples.

Also, if returning the minimum separation is removed, I'd appreciate a suggestion as to how one automatically adjusts nodes, and text as the graph changes.

As before, to reduce back and forth, feel free to change whatever you want.

@rocky
Copy link
Contributor Author

rocky commented Dec 30, 2020

Would you mind adding an example to the gallery in the "Drawing" section? Just create a new file examples/drawing/plot_<something>.py. You can look at the other examples to see you to do it, but basically all you need to do is create a docstring at the top of the file with the title of the plot for the gallery and then write a Python script that draws a tree using the hierarchical layout.

I recently used multipartite_layout to draw a circuit:
https://github.com/networkx/networkx/blob/master/examples/algorithms/plot_circuits.py

At the time, I wanted a more tree-like layout with the root at the top. Would you mind converting that example to use the hierarchical layout as well?

@jarrodmillman It just occurred to me how to do that. Basically the process would be:

  1. create tree edges
  2. call the heirarchical format routine to do the pos layout
  3. add non-tree edges
  4. graph with the pos positions returned in step 2.

Perhaps sometime in the new year I'll try this. But using the examples you requested before, I think it should be pretty straightforward for you or anyone else to try as well.

Base automatically changed from master to main March 4, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants