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

the laplacian_matrix method has a different dir parameter default than in Graphs.jl, making the doc confusing #24

Open
kragol opened this issue Jun 15, 2022 · 2 comments
Labels
breaking Fixing this would require a breaking change

Comments

@kragol
Copy link

kragol commented Jun 15, 2022

The laplacian_matrix function for SimpleWeightedDigraph uses dir=:out as default for the dir parameter. This contrasts with the definition in Graphs.jl where dir=:both is the default for directed graphs.

While I may agree that dir=:out could be a saner default, the problem is that the documentation of the function is only present in the Graphs package and mentions that the default is dir=:both for directed graphs. After looking at the doc at the REPL, the user may expect the default to be dir=:both when it is in fact dir=:out for SimpleWeightedDigraph.

@kragol kragol changed the title the laplacian_matrix method has a different dir parameter default than in Graphs.jl, making the doc confusing the laplacian_matrix method has a different dir parameter default than in Graphs.jl, making the doc confusing Jun 15, 2022
@matbesancon
Copy link
Member

I agree the inconsistency isn't great here. Feel free to open a PR to standardize on dir=:both here

@gdalle gdalle added the documentation Improvements or additions to documentation label Apr 6, 2023
@gdalle
Copy link
Member

gdalle commented Apr 6, 2023

Changing the default parameter would be breaking, instead I'm just adding this note to the docstring

@gdalle gdalle added breaking Fixing this would require a breaking change and removed documentation Improvements or additions to documentation labels Apr 6, 2023
gdalle added a commit that referenced this issue Apr 6, 2023
…#41)

Partially solves #24 without breaking backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Fixing this would require a breaking change
Projects
None yet
Development

No branches or pull requests

3 participants