Proposal: update semantics for nonisomorphic trees with order 0 or 1#8083
Conversation
Peiffap
left a comment
There was a problem hiding this comment.
The order==1 case is wrong I think.
Other than that: I see your nit_list and I raise you... my list of nits.
| @pytest.mark.parametrize("n", range(5)) | ||
| def test_nonisomorphic_tree_low_order_agreement(n): | ||
| """Ensure all the order<2 'special cases' are consistent.""" | ||
| assert len(list(nx.nonisomorphic_trees(n))) == nx.number_of_nonisomorphic_trees(n) |
There was a problem hiding this comment.
Nit, and it doesn't matter here, but since Ale just mentioned it it's fresh in my mind. We can avoid loading the whole list in memory.
| assert len(list(nx.nonisomorphic_trees(n))) == nx.number_of_nonisomorphic_trees(n) | |
| assert sum(1 for _ in nx.nonisomorphic_trees(n)) == nx.number_of_nonisomorphic_trees(n) |
There was a problem hiding this comment.
The memory footprint is negligible here, and I personally prefer the former pattern for readability when memory/performance is not a consideration. Just my 2c though!
Co-authored-by: Gilles Peiffer <gilles.peiffer.yt@gmail.com>
dschult
left a comment
There was a problem hiding this comment.
This looks good to me -- raise for negative order, yield nothing (raise StopIteration) for zero order. Use len(list(...)) instead of sum(1 for _ in ...)
Thanks for clearing up the corner cases!
Peiffap
left a comment
There was a problem hiding this comment.
All punctuation or style recs, take 'em or leave 'em. LGTM!
| >>> seen = [] | ||
| >>> for G in nx.nonisomorphic_trees(n): | ||
| ... assert not any(nx.is_isomorphic(G, H) for H in seen) | ||
| ... seen.append(G) |
There was a problem hiding this comment.
This avoids having to use seen, especially since we already have nit_list. Not necessarily better, your call!
| >>> seen = [] | |
| >>> for G in nx.nonisomorphic_trees(n): | |
| ... assert not any(nx.is_isomorphic(G, H) for H in seen) | |
| ... seen.append(G) | |
| >>> for i, G in enumerate(nit_list): | |
| ... assert not any(nx.is_isomorphic(G, H) for H in nit_list[i + 1:]) |
Co-authored-by: Gilles Peiffer <gilles.peiffer.yt@gmail.com>
|
This has had many eyes and a fair amount of time -- discussion has all been in agreement and 2 approvals (and the author makes a 2nd core approval). |
…etworkx#8083) * TST: Add tests for proposed semantics. * Implement proposed semantics. * DOC: Update documentation for noniso tree fns. * Apply suggestions from code review Co-authored-by: Gilles Peiffer <gilles.peiffer.yt@gmail.com> * Apply suggestions from code review Co-authored-by: Gilles Peiffer <gilles.peiffer.yt@gmail.com> --------- Co-authored-by: Gilles Peiffer <gilles.peiffer.yt@gmail.com>
While putting together an NX guide, I noticed a few (what I believe to be) inconsistencies in
nonisomorphic_treesandnumber_of_nonisomorphic_trees, specifically forn = 0andn = 1.Currently, these functions simply raise a
ValueError(with no exception message!) whenn < 2. However, I think this is inconsistent with the definition of a tree, both generally and as used elsewhere in the library. For example, a graph with a single node and no edges is technically a tree:is_treeraises aPointlessConceptexception for the null graph (i.e. no nodes or edges), which makes sense. However, I don't thinknx.number_of_nonisomorphic_treesshould raise in that case (nornx.nonisomorphic_trees, though this is subjective - see inline comment for details).Proposed semantics
I propose to change the behavior of
nx.nonisomorphic_treesandnx.number_of_nonisomorphic_treesfororder=0andorder=1. I went about this in a test-driven manner, so the first commit 9de48f2 captures the proposed changes most clearly (theorder < 0case is consistent with current behavior, albeit with an added message to theValueError)