Skip to content

Fix vertices(G) to return all vertices, including isolated ones#4633

Merged
benlorenz merged 1 commit intomasterfrom
bl/graphverts
Feb 24, 2025
Merged

Fix vertices(G) to return all vertices, including isolated ones#4633
benlorenz merged 1 commit intomasterfrom
bl/graphverts

Conversation

@benlorenz
Copy link
Copy Markdown
Member

fixes #4426
cc: @YueRen, @bkholler

The tests in algebraic statistics all seem to pass even with this change, so I don't think we need vertices_with_positive_degree.

@benlorenz benlorenz requested a review from lkastner February 24, 2025 09:10
@lgoettgens lgoettgens added topic: combinatorics release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Feb 24, 2025
Comment thread test/Combinatorics/Graph.jl
@lgoettgens
Copy link
Copy Markdown
Member

The tests in algebraic statistics all seem to pass even with this change, so I don't think we need vertices_with_positive_degree.

IIUC, their graphs are all connected, and thus in particular have no isolated vertices.

@afkafkafk13
Copy link
Copy Markdown
Collaborator

The tests in algebraic statistics all seem to pass even with this change, so I don't think we need vertices_with_positive_degree.

IIUC, their graphs are all connected, and thus in particular have no isolated vertices.

Should there be one (small) testing case with an isolated vertex to prevent future regression? (Given that the authors did not come across the corner case by themselves, this might be a sensible idea.)

@benlorenz
Copy link
Copy Markdown
Member Author

The tests in algebraic statistics all seem to pass even with this change, so I don't think we need vertices_with_positive_degree.

IIUC, their graphs are all connected, and thus in particular have no isolated vertices.

Should there be one (small) testing case with an isolated vertex to prevent future regression? (Given that the authors did not come across the corner case by themselves, this might be a sensible idea.)

This new testcase for vertices has many isolated vertices:

g = Graph{Directed}(5)
@test n_vertices(g) == 5
@test n_edges(g) == 0
@test vertices(g) == 1:5

In Oscar 1.2.2:

julia> g = Graph{Directed}(5)
Directed graph with 5 nodes and no edges

julia> vertices(g)
ERROR: ArgumentError: reducing over an empty collection is not allowed; consider supplying `init` to the reducer

@afkafkafk13
Copy link
Copy Markdown
Collaborator

This new testcase for vertices has many isolated vertices:

g = Graph{Directed}(5)
@test n_vertices(g) == 5
@test n_edges(g) == 0
@test vertices(g) == 1:5

Thank you! I had overlooked that one. No further comments from me.

@benlorenz benlorenz merged commit 892c895 into master Feb 24, 2025
@benlorenz benlorenz deleted the bl/graphverts branch February 24, 2025 17:06
@fingolfin fingolfin changed the title Graphs: fix vertices(G) to return all vertices, including isolated ones Fix vertices(G) to return all vertices, including isolated ones Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: combinatorics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Problems with vertices(::Graph) defined in AlgebraicStatistics

3 participants