Naive lowest common ancestor implementation

[dtekinoglu]

No description provided.

[dschult]

Looks like nx.ancestors has two arguments: nx.ancestors(G, v)

[dschult]

I went through the code and made some suggestions. Looks good!

[dschult]

@rossbar do you have any magic to handle parametrizing this test method?

[rossbar]

I would say that for this test it makes more sense to parametrize on the inputs since some of the tested objects are generators (i.e. some require next in order to trigger there behavior) and some are functions. For example, I'd probably write this test like so:

@pytest.mark.parametrize("graph_type", (nx.Graph, nx.MultiGraph, nx.MultiDiGraph))
def test_not_implemented(self, graph_type):
    G = graph_type([(0, 1)])
    with pytest.raises(nx.NetworkxNotImplemented):
        next(tree_all_pairs_lca(G))
    with pytest.raises(nx.NetworkxNotImplemented):
        next(all_pairs_lca(G))
    with pytest.raises(nx.NetworkxNotImplemented):
        nx.lowest_common_ancestor(G, 0, 1)

... and if you wanted to add more generators/functions, you could add more with statements (e.g.

with pytest.raises(nx.NetworkxNotImplemented):
    nx.naive_lowest_common_ancestor(G, 0, 1)

The other thing you could do is split this up into two separate tests, one for generators and one for functions, that would allow you to stacked parametrizations (see the end of this section). That might look something like:

@pytest.mark.parametrize("graph_type", (nx.Graph, nx.MultiGraph, nx.MultiDiGraph))
@pytest.mark.parametrize("lca_gen", (tree_all_pairs_lca, all_pairs_lca))
def test_not_implemented_generator(self, graph_type, lca_gen):
    G = graph_type([(0, 1)])
    with pytest.raises(nx.NetworkxNotImplemented):
        next(lca_gen(G))

@pytest.mark.parametrize("graph_type", (nx.Graph, nx.MultiGraph, nx.MultiDiGraph))
@pytest.mark.parametrize("lca_fn", (tree_all_pairs_lca, all_pairs_lca))
def test_not_implemented_function(self, graph_type, lca_fn):
    G = graph_type([(0, 1)])
    with pytest.raises(nx.NetworkxNotImplemented):
        lca_fn(G, 0, 1)

The trick with parametrizing tests (IMO) is to strike a balance between utility and readability - of course, where that balance lies is usually pretty subjective :)

[dtekinoglu]

Ready for review @MridulS @dschult @rossbar

[MridulS]

This looks great @dtekinoglu! added some little nit picks :)

[MridulS]

Not too sure if this check is required. Even if pairs isn't a list (and just an iterator), the for loop below should work just fine.

[dtekinoglu]

If I don't include this check test_naive_all_pairs_lowest_common_ancestor3 fails and I cannot figure out what else to do.

[MridulS]

Ahhh yes, this is happening because the iterator is being consumed in this checking loop. So the next loop over pairs ends up just being a loop over an empty iterator. Maybe we shouldn't even check this at all here. If nx.ancestors is used with a node which is not in the graph it gives a nice clean error, something like

....
....

NetworkXError: The node 1100 is not in the digraph.

What do you think about removing this else condition here?

[MridulS]

Something like this https://github.com/networkx/networkx/pull/5736/files#r914724794

[MridulS]

This is great! Thanks @dtekinoglu :)

[dschult]

This looks good to me! I have some comments below about possibly different ways of arranging loops in python. See what you think. None are critical. If you'd like the current code just reply with that and we'll merge it. :}

[rossbar]

Excellent, thanks @dtekinoglu ! This LGTM, just a stray comment about the tests.

[rossbar]

If I'm not missing anything, it looks like the G=None arg isn't used in any of the following tests. If so, then this check could be made easier to understand by removing the unused kwarg and associated scaffolding, e.g.

Suggested change

	def assert_lca_dicts_same(self, d1, d2, G=None):
	"""Checks if d1 and d2 contain the same pairs and
	have a node at the same distance from root for each.
	If G is None use self.DG."""
	if G is None:
	G = self.DG
	root_distance = self.root_distance
	else:
	roots = [n for n, deg in G.in_degree if deg == 0]
	assert len(roots) == 1
	root_distance = nx.shortest_path_length(G, source=roots[0])
	for a, b in ((min(pair), max(pair)) for pair in chain(d1, d2)):
	assert (
	root_distance[get_pair(d1, a, b)] == root_distance[get_pair(d2, a, b)]
	)
	def assert_lca_dicts_same(self, d1, d2):
	"""Checks if d1 and d2 contain the same pairs and
	have a node at the same distance from root for each.
	"""
	for a, b in ((min(pair), max(pair)) for pair in chain(d1, d2)):
	assert (
	self.root_distance[get_pair(d1, a, b)] ==
	self.root_distance[get_pair(d2, a, b)]
	)

[rossbar]

Ah - so I just looked at the rest of the file and realized that this method (along with most of the other patterns, e.g. the test method names) were taken from the existing test classes, which makes total sense. I think there's actually a lot to clean up in the existing tests, so let's not worry about this here... I will create a followup issue so that we can handle this separately and not block this PR.

I'm going to switch my previous comment to an approval, sorry for the noise @dtekinoglu !