-
Notifications
You must be signed in to change notification settings - Fork 93
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
Uniform code style #190
Merged
Merged
Uniform code style #190
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
af1f18f
Fix typos
aurorarossi b6b7797
Fix comment typo
aurorarossi 8b5c735
Housekeeping
aurorarossi e19f28b
Add and remove spaces
aurorarossi ff60f32
Update src/dominatingset/minimal_dom_set.jl
aurorarossi 1ee81d1
Apply JuliaFormatter and add it (+ Aqua) to testing
gdalle 40c420a
Fix formatting fail on Windows
gdalle 6170376
Update src/SimpleGraphs/generators/randgraphs.jl
gdalle 30f2539
Fix contributing guidelines + add Blue style and formatting
gdalle ab7212e
Fix formatting bug in vf2.jl by ignoring function
gdalle 00393ec
Fix ignore vf2.jl for formatting on Windows
gdalle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
style = "blue" | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,23 +5,32 @@ Following these guidelines will reduce friction and improve the speed at which y | |
|
||
## Bug reports | ||
|
||
If you notice code that crashes, is incorrect, or is too slow, please file a bug report. The report should be raised as a github issue with a minimal working example that reproduces the condition. | ||
If you notice code that crashes, is incorrect, or is too slow, please file a bug report. The report should be raised as a GitHub issue with a minimal working example that reproduces the condition. | ||
The example should include any data needed. If the problem is incorrectness, then please post the correct result along with an incorrect result. | ||
|
||
Please include version numbers of all relevant libraries and Julia itself. | ||
|
||
## Development guidelines | ||
|
||
- Correctness is a necessary requirement; efficiency is desirable. Once you have a correct implementation, make a Pull Request (PR) so we can help improve performance. | ||
- PRs should contain one logical enhancement to the codebase. | ||
- Squash commits in a PR. | ||
- If you want to introduce a new feature, open an issue to discuss a feature before you start coding (this maximizes the likelihood of patch acceptance). | ||
- Minimize dependencies on external packages, and avoid introducing new dependencies that would increase the compilation time by a lot. | ||
Here are a few principles to keep in mind when writing a Pull Request (PR). | ||
|
||
### Correctness | ||
|
||
- Correctness is a necessary requirement. Add tests to make sure that any new function displays the right behavior. | ||
- Since Graphs.jl supports multiple implementations of the graph data structure using the `AbstractGraph` [type](https://juliagraphs.github.io/Graphs.jl/latest/types.html#AbstractGraph-Type-1), you should refrain from using the internal fields of structs such as `fadjlist`. Instead, you should use the functions provided in the API. Code that is instrumental to defining a concrete graph type can use the internal structure of that type. | ||
- Put type assertions on all function arguments where conflict may arise (use abstract types, `Union`, or `Any` if necessary). | ||
- If the algorithm was presented in a paper, include a reference to the paper (_e.g._, a proper academic citation along with an eprint link). | ||
- Take steps to ensure that code works correctly and efficiently on edge cases (disconnected graphs, empty graphs, ...). | ||
- We can accept code that does not work for directed graphs as long as it comes with an explanation of what it would take to make it work for directed graphs. | ||
- Prefer the short circuiting conditional over `if`/`else` when convenient, and where state is not explicitly being mutated (*e.g.*, `condition && error("message")` is good; `condition && i += 1` is not). | ||
|
||
### Style | ||
|
||
- Write your code using Invenia's [BlueStyle](https://github.com/invenia/BlueStyle) | ||
- Format it with [JuliaFormatter](https://github.com/domluna/JuliaFormatter.jl) before pushing | ||
|
||
### Efficiency | ||
|
||
- Once you have a correct implementation, make a PR so we can help improve performance. | ||
- Minimize dependencies on external packages, and avoid introducing new dependencies that would increase the compilation time by a lot. | ||
- Write code to reuse memory wherever possible. For example: | ||
|
||
```julia | ||
|
@@ -34,7 +43,9 @@ function f(g, v) | |
return sum(storage) | ||
end | ||
``` | ||
|
||
should be rewritten as two functions | ||
|
||
```julia | ||
function f(g::AbstractGraph, v::Integer) | ||
storage = Vector{Int}(undef, nv(g)) | ||
|
@@ -49,45 +60,50 @@ function f!(g::AbstractGraph, v::Integer, storage::AbstractVector{Int}) | |
return sum(storage) | ||
end | ||
``` | ||
|
||
This gives users the option of reusing memory and improving performance. | ||
|
||
### Minimizing use of internal struct fields | ||
### Misc | ||
|
||
Since Graphs supports multiple implementations of the graph data structure using the `AbstractGraph` [type](https://juliagraphs.github.io/Graphs.jl/latest/types.html#AbstractGraph-Type-1), you should refrain from using the internal fields of structs such as `fadjlist`. Instead, you should use the functions provided in the API. | ||
Code that is instrumental to defining a concrete graph type can use the internal structure of that type. | ||
- If the algorithm was presented in a paper, include a reference to the paper (_e.g._, a proper academic citation along with an eprint link). | ||
|
||
## Git usage | ||
## Git(Hub) usage | ||
|
||
### Getting started on a package contribution | ||
|
||
In order to make it easier for you to contribute and review Pull Requests (PRs), | ||
it would be better to be familiar with git fundamentals. | ||
|
||
In order to make it easier for you to contribute and review PRs, it would be better to be familiar with Git fundamentals. | ||
Most importantly: | ||
|
||
- clone the repository from JuliaGraphs/Graphs.jl | ||
- fork the repository on your own github account | ||
- fork the repository on your own GitHub account | ||
- make the modification to the repository, test and document all your changes | ||
- push to the fork you created | ||
- open a PR. | ||
|
||
See the [JuMP documentation](https://jump.dev/JuMP.jl/dev/developers/contributing/) for a more detailed guide. | ||
|
||
### Advanced: visualize opened PRs locally: | ||
### PR hygiene | ||
|
||
- PRs should contain one logical enhancement to the codebase. | ||
- Squash commits in a PR. | ||
- If you want to introduce a new feature, open an issue to discuss a feature before you start coding (this maximizes the likelihood of patch acceptance). | ||
|
||
### Advanced: visualize opened PRs locally | ||
|
||
In order to make it easier for you to review Pull Requests (PRs), you can add this to your git config file, which should be located at `PACKAGE_LOCATION/.git/config`, where `PACKAGE_LOCATION` is where the Graphs.jl was cloned. | ||
In order to make it easier for you to review PRs, you can add this to your git config file, which should be located at `PACKAGE_LOCATION/.git/config`, where `PACKAGE_LOCATION` is where the Graphs.jl was cloned. | ||
If you added the package with the `] dev` command, it is likely at `$HOME/.julia/dev/Graphs`. | ||
|
||
These instructions were taken from [this gist](https://gist.github.com/piscisaureus/3342247). | ||
|
||
Locate the section for your github remote in the `.git/config` file. It looks like this: | ||
Locate the section for your GitHub remote in the `.git/config` file. It looks like this: | ||
|
||
``` | ||
[remote "origin"] | ||
fetch = +refs/heads/*:refs/remotes/origin/* | ||
url = [email protected]:JuliaGraphs/Graphs.jl.git | ||
``` | ||
|
||
Now add the line `fetch = +refs/pull/*/head:refs/remotes/origin/pr/*` to this section. Obviously, change the github url to match your project's URL. It ends up looking like this: | ||
Now add the line `fetch = +refs/pull/*/head:refs/remotes/origin/pr/*` to this section. Obviously, change the GitHub URL to match your project's URL. It ends up looking like this: | ||
|
||
``` | ||
[remote "origin"] | ||
|
@@ -96,7 +112,7 @@ Now add the line `fetch = +refs/pull/*/head:refs/remotes/origin/pr/*` to this se | |
fetch = +refs/pull/*/head:refs/remotes/origin/pr/* | ||
``` | ||
|
||
Now fetch all the pull requests: | ||
Now fetch all the PRs: | ||
|
||
``` | ||
$ git fetch origin | ||
|
@@ -108,7 +124,7 @@ From github.com:JuliaGraphs/Graphs.jl | |
... | ||
``` | ||
|
||
To check out a particular pull request: | ||
To check out a particular PR: | ||
|
||
``` | ||
$ git checkout pr/999 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,18 @@ | ||
using BenchmarkTools | ||
using Graphs | ||
|
||
|
||
DIGRAPHS = Dict{String,DiGraph}( | ||
"complete100" => complete_digraph(100), | ||
"path500" => path_digraph(500) | ||
"complete100" => complete_digraph(100), "path500" => path_digraph(500) | ||
) | ||
|
||
GRAPHS = Dict{String,Graph}( | ||
"complete100" => complete_graph(100), | ||
"tutte" => smallgraph(:tutte), | ||
"path500" => path_graph(500) | ||
"complete100" => complete_graph(100), | ||
"tutte" => smallgraph(:tutte), | ||
"path500" => path_graph(500), | ||
) | ||
|
||
|
||
suite = BenchmarkGroup() | ||
include("core.jl") | ||
|
||
|
||
tune!(suite); | ||
results = run(suite, verbose = true, seconds = 10) | ||
results = run(suite; verbose=true, seconds=10) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,26 @@ | ||
|
||
@benchgroup "centrality" begin | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): degree" Graphs.degree_centrality($g) | ||
@bench "$(name): closeness" Graphs.closeness_centrality($g) | ||
if nv(g) < 1000 | ||
@bench "$(name): betweenness" Graphs.betweenness_centrality($g) | ||
@bench "$(name): katz" Graphs.katz_centrality($g) | ||
end | ||
end | ||
end #graphs | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): degree" Graphs.degree_centrality($g) | ||
@bench "$(name): closeness" Graphs.closeness_centrality($g) | ||
if nv(g) < 1000 | ||
@bench "$(name): betweenness" Graphs.betweenness_centrality($g) | ||
@bench "$(name): katz" Graphs.katz_centrality($g) | ||
end | ||
if nv(g) < 500 | ||
@bench "$(name): pagerank" Graphs.pagerank($g) | ||
end | ||
end | ||
end # digraphs | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): degree" Graphs.degree_centrality($g) | ||
@bench "$(name): closeness" Graphs.closeness_centrality($g) | ||
if nv(g) < 1000 | ||
@bench "$(name): betweenness" Graphs.betweenness_centrality($g) | ||
@bench "$(name): katz" Graphs.katz_centrality($g) | ||
end | ||
end | ||
end # graphs | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): degree" Graphs.degree_centrality($g) | ||
@bench "$(name): closeness" Graphs.closeness_centrality($g) | ||
if nv(g) < 1000 | ||
@bench "$(name): betweenness" Graphs.betweenness_centrality($g) | ||
@bench "$(name): katz" Graphs.katz_centrality($g) | ||
end | ||
if nv(g) < 500 | ||
@bench "$(name): pagerank" Graphs.pagerank($g) | ||
end | ||
end | ||
end # digraphs | ||
end # centrality |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
@benchgroup "connectivity" begin | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): strongly_connected_components" Graphs.strongly_connected_components($g) | ||
end | ||
end # digraphs | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): connected_components" Graphs.connected_components($g) | ||
end | ||
end # graphs | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): strongly_connected_components" Graphs.strongly_connected_components( | ||
$g | ||
) | ||
end | ||
end # digraphs | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): connected_components" Graphs.connected_components($g) | ||
end | ||
end # graphs | ||
end # connectivity |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
@benchgroup "insertions" begin | ||
n = 10000 | ||
@bench "ER Generation" g = SimpleGraph($n, 16 * $n) | ||
n = 10000 | ||
@bench "ER Generation" g = SimpleGraph($n, 16 * $n) | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,14 @@ | ||
@benchgroup "traversals" begin | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): bfs_tree" Graphs.bfs_tree($g, 1) | ||
@bench "$(name): dfs_tree" Graphs.dfs_tree($g, 1) | ||
end | ||
end # graphs | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): bfs_tree" Graphs.bfs_tree($g, 1) | ||
@bench "$(name): dfs_tree" Graphs.dfs_tree($g, 1) | ||
end | ||
end # digraphs | ||
@benchgroup "graphs" begin | ||
for (name, g) in GRAPHS | ||
@bench "$(name): bfs_tree" Graphs.bfs_tree($g, 1) | ||
@bench "$(name): dfs_tree" Graphs.dfs_tree($g, 1) | ||
end | ||
end # graphs | ||
@benchgroup "digraphs" begin | ||
for (name, g) in DIGRAPHS | ||
@bench "$(name): bfs_tree" Graphs.bfs_tree($g, 1) | ||
@bench "$(name): dfs_tree" Graphs.dfs_tree($g, 1) | ||
end | ||
end # digraphs | ||
end # traversals |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlueStyle seems like the most well-established specification, whereas SciML is more recent. However JuliaFormatter cannot do it all, so we must tell contributors to adhere to the style guidelines in general. Should we add a badge to the README too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to prefer
BlueStyle
? (I am not yet familiar with these styles). We could also specify our own rules right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any cases, most of the changes here seem to be sensible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just use it because it's already well documented, so we can point contributors to the exhaustive specification by Invenia. We could make up our own rules but that would require explaining them to everyone in our docs.
Also, it's easier to contribute when you're already familiar with the style guide, which wouldn't be the case for Graphs-specific rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hot take is that the precise choice of style does not matter, as long as it's clear and easy to apply, and BlueStyle satisfies both criteria
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense to use some common style, as then people don't need to reconfigure their editor differently for every project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the default format by JuliaFormatter? Would that also be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default format by JuliaFormatter is much less well-specified than BlueStyle. One of the perks of BlueStyle is that it also gives coherent writing advice to the contributors that cannot be enfored by a simple linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another perk of JuliaFormatter is that the formatting in the Julia extension for VSCode is based on it. And it takes into account the
.JuliaFormatter.toml
file at the root of the package to format each file according to the local specification (in our casestyle = "blue"
)