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

Combinatorics: fix degree(::Graph) docu #4441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Combinatorics: fix degree(::Graph) docu #4441

wants to merge 1 commit into from

Conversation

YueRen
Copy link
Member

@YueRen YueRen commented Jan 10, 2025

Addresses #4440

@YueRen
Copy link
Member Author

YueRen commented Jan 10, 2025

Could somebody from the polymake team please check whether the changes are okay? If not, feel free to push changes to the branch. Many thanks in advance!

Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

The docstring states If the graph is directed, the neighbors reachable via outgoing edges are returned., so the current behaviour of neighbors is as documented.

Maybe @benlorenz or @lkastner have some opinion on this?

@lgoettgens
Copy link
Member

Sorry for the confusion. My comment was about the function neighbors that you modified. This functions states If the graph is directed, the neighbors reachable via outgoing edges are returned. in its docstring and may thus not be changed in this way.
This would then just mean that you would need to find a different way to patch degree

@YueRen
Copy link
Member Author

YueRen commented Jan 10, 2025

Okay, I think I got confused. The neighbors docstring clearly states that only out-neighbors are counted, however the degree docstring (which is how I stumbled upon this issue) does not.

Should we
(a) change the neighbors function so it counts both out- and in-neighbours as in the pull-request (+ update the documentation accordingly)
(b) change the degree function so it counts both out- and in-neighbours
(c) update the degree documentation so it warns the use that only out-neighbours are counted.

@YueRen YueRen marked this pull request as draft January 10, 2025 12:00
@benlorenz
Copy link
Member

benlorenz commented Jan 10, 2025

The docstring for neighbors (which you are modifying here) does have this. Unfortunately it is not mentioned for degree.

The neighbors function does match what julia Graphs.jl does: https://juliagraphs.org/Graphs.jl/v1.5/core/#Graphs.neighbors-Tuple{AbstractGraph,%20Integer} (defaulting to outneighbors)
But we chose degree to be consistent with neighbors, while Graphs.jl counts all edges in that case.

In any case these are decisions that were made and it is not a clear bugfix. So we cannot change that behavior until 2.0.
I would suggest adding a note to the documentation of degree.
In hindsight it would have been better to disallow these for directed graphs to avoid confusion.

Edit: We do also have all_neighbors, not sure how one would call a corresponding function for degree though.

@YueRen YueRen changed the title Combinatorics: fix neighbors(::Graph,::Int64) Combinatorics: fix degree(::Graph) docu Jan 10, 2025
@YueRen
Copy link
Member Author

YueRen commented Jan 10, 2025

Okay, I've changed the commit. It now only adds a comment to the docu of degree.

@YueRen YueRen marked this pull request as ready for review January 10, 2025 13:04
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.36%. Comparing base (43499a7) to head (658ea33).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4441      +/-   ##
==========================================
- Coverage   84.39%   84.36%   -0.04%     
==========================================
  Files         663      663              
  Lines       87938    87938              
==========================================
- Hits        74214    74187      -27     
- Misses      13724    13751      +27     
Files with missing lines Coverage Δ
src/Combinatorics/Graphs/functions.jl 96.72% <ø> (ø)

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants