Repository: networkx/networkx
Pull request: Check that nodes have "pos" attribute in geometric_edges
Closes #5046 .

Adds input validation that raises a more clear exception in the case where any node in the input graph does not have a `"pos"` attribute, which is required by the `geometric_edges` function.

I structured this in such a way that the input validation is only done in the user-facing function so that it doesn't add overhead to all of the other geometric graph generators.

Problem statement:
Should `geometric_edges` raise a specific exception if nodes don't have "pos" attribute?
The `networkx.generators.geometric.geometric_edges` function assumes that the input graph contains nodes that have a `"pos"` attribute representing the 2D position of the node. If this is not the case, exceptions are raised (depending on whether or not scipy is installed) that don't really indicate what the problem is:

```python
>>> G = nx.path_graph(3)
```

### with scipy

```python
>>> nx.generators.geometric.geometric_edges(G, radius=1, p=2)
Traceback (most recent call last)
   ...
ckdtree.pyx in scipy.spatial.ckdtree.cKDTree.__init__()

ValueError: data must be 2 dimensions
```
### without scipy
```python
>>> nx.generators.geometric.geometric_edges(G, radius=1, p=2)
Traceback (most recent call last):
   ...
TypeError: 'NoneType' object is not iterable
```

The thing that makes the most sense to me would be to raise an exception with a specific message to indicate to users that the nodes need a "pos" attribute. Another potential solution would be to have a default value for position when the attribute isn't found, but I don't know what a sensible default would be (`(0, 0)`)?

Hints from issue discussion:
I would say it should raise the exception. Putting a bunch of nodes at (0,0) would be potentially hard to notice.
Does raising the exception mean we should go through every node to check if it has a 2-tuple value on a `pos` attribute?
Or is it better to catch the current error and provide a better message for it? 
>  Does raising the exception mean we should go through every node to check if it has a 2-tuple value on a pos attribute?

This is what I was thinking just because it's fewer things to modify (adding one check at the beginning versus catching exceptions in multiple branches). The latter may actually be preferable from a performance perspective however since `geometric_edges` is used internally in a couple of the geometric generators where an extra check for node attributes would be unnecessary, and would actually slow things down.

Commit message: Add input validation to geometric_edges.

Patch to review:
diff --git a/networkx/generators/geometric.py b/networkx/generators/geometric.py
index 49996447cc0..817386929d9 100644
--- a/networkx/generators/geometric.py
+++ b/networkx/generators/geometric.py
@@ -84,6 +84,25 @@ def geometric_edges(G, radius, p=2):
     >>> nx.geometric_edges(G, radius=9)
     [(0, 1), (0, 2), (1, 2)]
     """
+    # Input validation - every node must have a "pos" attribute
+    for n, pos in G.nodes(data="pos"):
+        if pos == None:
+            raise nx.NetworkXError(
+                f"All nodes in `G` must have a 'pos' attribute. Check node {n}"
+            )
+
+    # NOTE: See _geometric_edges for the actual implementation. The reason this
+    # is split into two functions is to avoid the overhead of input validation
+    # every time the function is called internally in one of the other
+    # geometric generators
+    return _geometric_edges(G, radius, p)
+
+
+def _geometric_edges(G, radius, p=2):
+    """
+    Implements `geometric_edges` without input validation. See `geometric_edges`
+    for complete docstring.
+    """
     nodes_pos = G.nodes(data="pos")
     try:
         import scipy as sp
@@ -189,7 +208,7 @@ def random_geometric_graph(n, radius, dim=2, pos=None, p=2, seed=None):
         pos = {v: [seed.random() for i in range(dim)] for v in G}
     nx.set_node_attributes(G, pos, "pos")
 
-    G.add_edges_from(geometric_edges(G, radius, p))
+    G.add_edges_from(_geometric_edges(G, radius, p))
     return G
 
 
@@ -315,7 +334,7 @@ def should_join(edge):
         dist = (sum(abs(a - b) ** p for a, b in zip(pos[u], pos[v]))) ** (1 / p)
         return seed.random() < p_dist(dist)
 
-    G.add_edges_from(filter(should_join, geometric_edges(G, radius, p)))
+    G.add_edges_from(filter(should_join, _geometric_edges(G, radius, p)))
     return G
 
 
@@ -778,7 +797,7 @@ def thresholded_random_geometric_graph(
 
     edges = (
         (u, v)
-        for u, v in geometric_edges(G, radius, p)
+        for u, v in _geometric_edges(G, radius, p)
         if weight[u] + weight[v] >= theta
     )
     G.add_edges_from(edges)
diff --git a/networkx/generators/tests/test_geometric.py b/networkx/generators/tests/test_geometric.py
index 8075939d69b..58490bf5181 100644
--- a/networkx/generators/tests/test_geometric.py
+++ b/networkx/generators/tests/test_geometric.py
@@ -2,6 +2,8 @@
 import random
 from itertools import combinations
 
+import pytest
+
 import networkx as nx
 
 
@@ -318,3 +320,10 @@ def test_theta(self):
             # Adjacent vertices must be within the given distance.
             if v in G[u]:
                 assert (G.nodes[u]["weight"] + G.nodes[v]["weight"]) >= 0.1
+
+
+def test_geometric_edges_raises_no_pos():
+    G = nx.path_graph(3)
+    msg = "All nodes in `G` must have a 'pos' attribute"
+    with pytest.raises(nx.NetworkXError, match=msg):
+        nx.geometric_edges(G, radius=1)


Code review comments:
diff_hunk: @@ -84,6 +84,25 @@ def geometric_edges(G, radius, p=2):
     >>> nx.geometric_edges(G, radius=9)
     [(0, 1), (0, 2), (1, 2)]
     """
+    # Input validation - every node must have a "pos" attribute
+    for n, pos in G.nodes(data="pos"):
+        if pos == None:
path: networkx/generators/geometric.py
text: @user1:
I guess this line could be `if pos is None:` by some style standards. (Maybe a small speed up, but not sure)

@author:
Fixed in 233bbc3!