Repository: networkx/networkx
Pull request: Error message improvement for nbunch_iter ( NetworkXError raised with specific message on TypeError with "iter" in msg )
This closes https://github.com/networkx/networkx/issues/7740

Changes :

    Error message changed to include what nbunch is instead of just 'nbunch'

    Tests changed accordingly to look for new error message.

    Small improvement in documentation && documentation changed accordingly.


Problem statement:
Opaque exception message for degree when node not present
I think there's an opportunity to improve the clarity of the exception raised when requesting the degree of a node not found in `G`:

```python
>>> G = nx.path_graph(5)
>>> G.degree(100)  # Node "100" not in G
Traceback (most recent call last):
  File "/home/ross/repos/networkx/networkx/classes/graph.py", line 2040, in bunch_iter
    for n in nlist:
             ^^^^^
TypeError: 'int' object is not iterable

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ross/repos/networkx/networkx/classes/reportviews.py", line 440, in __call__
    return self.__class__(self._graph, nbunch, weight)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ross/repos/networkx/networkx/classes/reportviews.py", line 425, in __init__
    self._nodes = self._succ if nbunch is None else list(G.nbunch_iter(nbunch))
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ross/repos/networkx/networkx/classes/graph.py", line 2055, in bunch_iter
    raise exc
networkx.exception.NetworkXError: nbunch is not a node or a sequence of nodes.
```

There are multiple messages fromt he chained exceptions, but neither really points to the real issue: the node is not in `G`.

Hints from issue discussion:
I've read through the relevant code. 


I think this happens because the line of code used to check for the case where nbunch is a single node,         
`elif nbunch in self:  # if nbunch is a single node` (in line 2004 of graph.py)
can only filter out single nodes that are in self, and single nodes that are not in self get tossed to the else statement later on. This works, but without the correct error message as mentioned.


One method of possibly solving this is to change/ add an elif statement that appropriately raises a NetworkXError "nbunch(or node) is not in self(or G/ sequence)."

I'm not familiar on the ways around this codebase, so I ask for advice on how I should solve this.
@rossbar 

I forgot to add this. 😢
I think this is an expectation mismatch issue.  `nbunch` can be either a node or a collection of nodes. The error message says: `nbunch is not a node or a sequence of nodes.` and that makes sense so long as you think of your input as an "nbunch".  The user says: OK, the nbunch I input is not a node or a sequence of nodes. I need to make it a node or sequence of nodes.  But if you think of your input as a node, you expect a message like `the node is not in G`.

`G.degree`  probably tries to do too many things. The function interface `G.degree(nbunch)` accepts an nbunch while the dict interface  `G.degree[node]` looks up a node. Most of the time, neither of these should be used, because we rarely want to look up the degree of only one node. Most code should just iterate over the `(node, degree)` pairs by using `G.degree`. If processing involves multiple lookups or comparing degrees, etc, then we should use `G_degree = dict(G.degree)` following by e.g. `G_degree[100]` because this avoids repeated use of the "degree view" lookup. dict construction and lookups are much faster than the python classes which implement views.

We could try making the docs clearing indicate that looking up the degree of individual nodes is not performant. we could remove the function interface (it is included mostly for the rare case when people want to iterate over the degree of a subset of the nodes -- it might be faster to use `(deg for node, deg in G.degree if node in nbunch)`, I don't know), Or we could change the error message in `nbunch_iter` to include the graph considered, e.g. `nbunch is not a node in G or a sequence of nodes.`   Thoughts?
@dschult We can also modify the `DegreeView.__getitem__()` to raise an exception when checking for missing nodes. 

```python
if n not in self._graph:
        raise NetworkXError(f"Node {n} not found in the graph")
```
Simultaneously, we could make an additional change by raising an error in the else statement of `nbunch_iter`. If no one has taken up this issue, I'm willing to make the changes!
@dschult @akshitasure12 Thank you for your reply. 

This is what I've understood :

1. G.degree does too much things in the first place, and it would be better in the long run to have dictionary construction && lookup replace view classes.

-- While removing the function interface is a way of solving that, we could also possibly nudge people to use dictionaries by providing a more convenient method. The function could do the construction && lookups for the users, but I can't come up with anything more right now.

2. We could change the error messages in nbunch_iter

I think we could possibly add a if block under `except TypeError as err:` to catch the following error :

`"TypeError: 'int' object is not iterable"`

Currently, this is included in this block of code :
``` 
                   if "iter" in message:
                        exc = NetworkXError(
                            "nbunch is not a node or a sequence of nodes."
                        )
```

We can modify it to this :

```
                except TypeError as err:
                    exc, message = err, err.args[0]
                    # capture error for non-sequence/iterator nbunch.
                    if "iter" in message:
                        exc = NetworkXError(
                            "nbunch is not a node or a sequence of nodes."
                        )
                    # capture error for integer nbunch that is not in G.
                    if "int" in message:
                        exc = NetworkXError(
                            f"Node {nbunch} is not in the graph."
                        )
```

The added block( the second one ) overrides the error message from the previous block.

I also think it's really neat since it can specify the node.

_However, further consideration is needed as the line_
`if "int" in message` might catch other unexpected errors. I want to know if there is something that rings a bell.
(Maybe adj could somehow become an integer and raise an error?)

For this, we would also have to modify the tests. ( I tested after the changes and got thrown with 12 failures )
ex) bunch = G.nbunch_iter(-1) would be catched better with the new message

+ Would there be any better error messages?

3. We could modify DegreeView for an error message

-- This is indeed valid, but as far as I have read, there weren't any other instances of error handling in this part of the codebase. I think it is in the realm of good practice to keep the error handling in designated places(in this case, nbunch_iter), since putting them all over would make maintenance more difficult. 

Sorry for the late reply. I caught a harsh cold && went on vacation for new year's.

+ @akshitasure12 , if you don't mind, I would like to wrap up this issue.
I like your suggestion 2), and would like to make that appear for types other than `int` where that makes sense.  The idea is to handle the TypeError message when `nbunch` is not iterable. We can then refer to it as a node. Maybe we can use something like:
```python
    if `object is not iterable` in message:
        exc = NetworkXError(f"Node {nbunch} is not in the graph")
```
I've read through the tests for nbunch in test_graph.py with the blame view.

"Node {nbunch} is not in the graph" looks perfectly good for the error message.


I think the original message "... not a sequence of nodes" was intended for the edge case which is catched by the second block(elif) and the NetworkXError that is thrown with the inclusion of "iter" in the error message.

Besides that, our error message is the equivalent of the original, only with added clarity. 

This seems to close #7740. I want to note the possibility of new functionality that uses dictionaries for faster lookups that was discussed here.


I will create a pull request that implements the changes that have been mentioned here. I will also update the relevant documentation && tests.
Don't replace the use of `DegreeView` with dict creation. (I'm not sure if that is what you intend by suggestion 1).

We need the function interface of `DegreeView` to allow specification of the `weight` argument. Maybe there is another api that would be cleaner, but I haven't come up with one.  We might be able to help most people having trouble by adding a note or warning in the docs that `G.degree(v, weight="wt")` is an inefficient way of looking at the degree of all nodes. Iterate like `for node, degree in G.degree(weight="wt")` or create a degree dict `degree = dict(G.degree(weight="wt"))`.

Commit message: Enhanced error message #7740

Patch to review:
diff --git a/networkx/classes/graph.py b/networkx/classes/graph.py
index 6828705d128..36e72d5e190 100644
--- a/networkx/classes/graph.py
+++ b/networkx/classes/graph.py
@@ -1993,7 +1993,7 @@ def nbunch_iter(self, nbunch=None):
         """Returns an iterator over nodes contained in nbunch that are
         also in the graph.
 
-        The nodes in nbunch are checked for membership in the graph
+        The nodes in an iterable nbunch are checked for membership in the graph
         and if not are silently ignored.
 
         Parameters
@@ -2047,6 +2047,11 @@ def bunch_iter(nlist, adj):
                         exc = NetworkXError(
                             "nbunch is not a node or a sequence of nodes."
                         )
+                    # capture single nodes that are not in the graph.
+                    if "object is not iterable" in message:
+                        exc = NetworkXError(
+                            f"Node {nbunch} is not in the graph."
+                        )
                     # capture error for unhashable node.
                     if "hashable" in message:
                         exc = NetworkXError(
diff --git a/networkx/classes/tests/test_graph.py b/networkx/classes/tests/test_graph.py
index b0048a31f04..3288b276503 100644
--- a/networkx/classes/tests/test_graph.py
+++ b/networkx/classes/tests/test_graph.py
@@ -139,7 +139,7 @@ def test_nbunch_iter(self):
         # node not in graph doesn't get caught upon creation of iterator
         bunch = G.nbunch_iter(-1)
         # but gets caught when iterator used
-        with pytest.raises(nx.NetworkXError, match="is not a node or a sequence"):
+        with pytest.raises(nx.NetworkXError, match="is not in the graph"):
             list(bunch)
         # unhashable doesn't get caught upon creation of iterator
         bunch = G.nbunch_iter([0, 1, 2, {}])


Code review comments:
diff_hunk: @@ -2047,6 +2047,11 @@ def bunch_iter(nlist, adj):
                         exc = NetworkXError(
                             "nbunch is not a node or a sequence of nodes."
                         )
+                    # capture single nodes that are not in the graph.
+                    if "object is not iterable" in message:
+                        exc = NetworkXError(
+                            f"Node {nbunch} is not in the graph."
+                        )
path: networkx/classes/graph.py
text: @user1:
```suggestion
                        exc = NetworkXError(f"Node {nbunch} is not in the graph.")
```