Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 random_ordered_tree and forest_str #4294
Add random_ordered_tree and forest_str #4294
Changes from 1 commit
e518a7d
3ec93c5
5af0be6
a95e69d
f948002
a0603c4
56c99e7
a6741bd
2c0503a
ea32250
f14a568
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You almost certainly don't need an
OrderedGraph
class here. That would be needed for Python version pre-3.6. But with Python3.6+ the regular classes are ordered because the underlying dict is ordered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is likely being removed as it is redundant with the new
create_using
arg inrandom_tree
.While dictionaries did becomes ordered by default in 3.6, that guarantee is still not part of the Python language spec AFAIK. I would much prefer to be explicit about the orderedness of graphs. Furthermore, there are mathematical implications of having an ordered graph versus one that has an arbitrary order. The algorithms in the PRs that build on this actually depend on the graph having a defined order for correctness and runtime analysis. I know there are randomized algorithms that depend on random orderings; I'm not sure if any rely on arbitrary orderings.
I'm stressing this to make sure the maintainers know there is value in explicitly using and keeping
Ordered
Graph/DiGraph/etc...
variants in the API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that comment.
Also, CPython 3.7+ guarantees the order of dicts to be the order keys are added. I guess if you try to use Pypy that is not guaranteed, but they are still on version2.7 and our current code probably is starting to break on that version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW pypy has a 2.7 and a 3.6 version.
I imagine order will be added to the Python dict spec in the future.(I now see you are saying that it was added in 3.7 into the language spec).Regardless, my main concern is that networkx maintains the ability to distinguish between a graph that is ordered and a graph that has arbitrary order for algorithmic purposes (even if that's not the implementation under the hood).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only testing on PyPy 3.6 now. I am waiting on this PR actions/setup-python#168 to be merged and I will switch us to PyPy 3.7. They have been working on getting the PyPy 3.7 support regularly for the last few weeks. So I expect this to happen soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only used
OrderedGraph
for testing in the past. I intended to propose deprecating it once we drop Python 3.6, which will happen very soon.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful about claiming alignment between OrderedGraph and a (mathematical) ordered graph. I don't think it is accurate to use these classes to distinguish between "a graph that is ordered and a graph that has arbitrary order for algorithmic purposes". Certainly it is important to distinguish them mathematically. But both OrderedGraph and Graph have the same deterministic node and edge reporting order based on the order that nodes and edges were added to the object.
We should deprecate OrderedGraph, though it might be useful to show an example of subclassing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is that the algorithm I implemented in #4327 explicitly requires an OrderedDiGraph, and I do check for that. But this is just an input type check, and I suppose given that a DiGraph is ordered under the hood, the algorithm will still work. I'm worried that without this check a user might think that they are getting the maximum embedding of unordered graphs (which is an APX-hard problem, and the algo I wrote does not find that). I recognize that functionally they are equivalent. I just believe that the semantics are important for implementation elegance.
That's my main argument for keeping ordered graph variants, and if others don't think its strong enough to justify its existence, then I'll accept that.
Regardless, this function and all references to the OrderedDiGraph and OrderedGraph class no longer exist in this PR. So perhaps we can move forward with merging this and save the deprecation discussion for another issue.