Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync up behavior of is_{type} for empty graphs #5849

Merged
merged 9 commits into from Dec 18, 2023

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Jul 6, 2022

Currently we get very different answers if we use a nx.is_something() method with an empty graph. This PR tries to sync them up. It's still not synced up, we should probably decide on one behavior.

Current:

  • is_aperiodic -> StopIteration (empty digraph)
  • is_arborescence -> NetworkXPointlessConcept: G has no nodes.
  • is_branching -> NetworkXPointlessConcept: G has no nodes.
  • is_chordal -> StopIteration
  • is_connected -> NetworkXPointlessConcept: ('Connectivity is undefined ', 'for the null graph.')
  • is_distance_regular -> StopIteration
  • is_eulerian -> NetworkXPointlessConcept: ('Connectivity is undefined ', 'for the null graph.')
  • is_forest -> NetworkXPointlessConcept: G has no nodes.
  • is_regular -> StopIteration
  • is_semiconnected -> NetworkXPointlessConcept: Connectivity is undefined for the null graph.
  • is_semieulerian -> NetworkXPointlessConcept: ('Connectivity is undefined ', 'for the null graph.')
  • is_strongly_connected -> NetworkXPointlessConcept: Connectivity is undefined for the null graph.
  • is_strongly_regular -> StopIteration
  • is_tree -> NetworkXPointlessConcept: G has no nodes.
  • is_weakly_connected -> NetworkXPointlessConcept: Connectivity is undefined for the null graph.

I have added a nx.NetworkXError("Graph has no nodes.") error for methods which were raising a StopIteration error. I could change this to NetworkXPointlessConcept if that makes more sense. Ideally I would like to see all of these methods returning uniform answers. Maybe there should be a decorator for this :me cringing:

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for fixing these exceptions. To summarize the discussion from the meeting:

  • Switch these to NetworkxPointlessConcept exceptions for now and/or
  • Introduce a new exception with a friendlier name (e.g. EmptyGraph?) that subclasses the pointless concept exception to use in cases like this.

It might make sense to introduce the new exception in this PR so that we don't forget to use it for these instances! Either way works though.

@MridulS
Copy link
Member Author

MridulS commented Dec 14, 2023

@rossbar updated all the functions to be synced up with the NetworkxPointlessConcept error.

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. You've changed 4 functions and that fixes 5 functions.

I know we talked about a more gentle name than pointlessconcept, but atm I am thinking EmptyGraph doesn't indicate that something is wrong. And I'm thinking if we change the name we will need to deprecate that. So unless there is a good alternative name, I'd say let's merge this and live with a slightly obnoxious name (that is very memorable and makes it clear that something is wrong with G being empty)

So, I approve this! Thanks @MridulS

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MridulS !

I went ahead and added tests for all the ones that weren't already tested - that way, the test suite will indicate if we're changing behavior for these corner cases in the future.

The only fn from the original list that doesn't raise on empty input is is_chordal, which returns True as of #6563. Perhaps we to revisit the behavior for the sake of consistency. Also, I noticed a nx.is_k_regular function that wasn't in the original listing which returns True for empty graphs (with any value of k) so that's another one to look at.

I vote to leave those for follow-up PRs though - this LGTM! I won't immediately merge to give others the chance to double-check my test additions.

@dschult
Copy link
Member

dschult commented Dec 18, 2023

Thanks @MridulS and @rossbar !
This looks good.

I think some is_* functions should return True or False for empty_graphs.
It depends on the trait that they are considering. Not all traits are pointless concepts for an empty graph. :}
But I could probably be convinced otherwise. :}

@rossbar rossbar merged commit 92afda9 into networkx:main Dec 18, 2023
39 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Dec 18, 2023
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
Improve consistency of property-checking functions (i.e. is_something)
for empty graph inputs.

Tend towards NetworkXPointlessConcept exceptions for consistency.

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants