Make HITS raise exceptions consistent with power iterations #7084
Conversation
Specifically, instead of raising `ValueError` or `sp.sparse.linalg.ArpackNoConvergence`, change `nx.hits` to raise `nx.PowerIterationFailedConvergence` just like other algorithms that have power iterations. This makes it easier for backends to raise consistent exceptions (`ArpackNoConvergence` is particularly awkward to match).
|
This is an API change. Should we do the deprecation workflow? |
|
No opinion from me, but I think you're right it is technically an API change. What's the networkx deprecation mechanism for this? Raise a warning and/or put a notice in the docstring that the exceptions will change? |
I'm inclined to see if we can get away without one. AFAICT this will only burn folks that are explicitly catching |
| if max_iter <= 0: | ||
| raise nx.PowerIterationFailedConvergence(max_iter) |
There was a problem hiding this comment.
Just a nit, but I think a ValueError may be better here. "Practicality over Purity" though, so if this is a huge pain for backends then not a blocker!
There was a problem hiding this comment.
PowerIterationFailedConvergence was chosen here to match pagerank, eigenvector_centrality, and katz_centrality. I agree ValueError can make sense (esp. for max_iter < 0), but I would want to update the other algorithms too.
There was a problem hiding this comment.
+1 for consistency then, thanks!
…#7084) Specifically, instead of raising `ValueError` or `sp.sparse.linalg.ArpackNoConvergence`, change `nx.hits` to raise `nx.PowerIterationFailedConvergence` just like other algorithms that have power iterations. This makes it easier for backends to raise consistent exceptions (`ArpackNoConvergence` is particularly awkward to match).
Specifically, instead of raising
ValueErrororsp.sparse.linalg.ArpackNoConvergence, changenx.hitsto raisenx.PowerIterationFailedConvergencejust like other algorithms that have power iterations. This makes it easier for backends to raise consistent exceptions (ArpackNoConvergenceis particularly awkward to match).