JuliaGraphs / MultilayerGraphs.jl

A Julia package for the creation, manipulation and analysis of the structure, dynamics and functions of multilayer graphs.
https://juliagraphs.org/MultilayerGraphs.jl/dev
MIT License
118 stars 5 forks source link

[review JOSS] #129

Closed bgailleton closed 1 year ago

bgailleton commented 1 year ago

This issue review concerns my Review for the related submission in JOSS: openjournals/joss-reviews#5116

Installation was straightforward, but I echo @TomKellyGenetics that there might be minor versioning conflicts. I managed to install it on Julia 1.8.6 (Ubuntu 22.04) but had to update Julia before. It is not a problem but the documentation should explicitly mention the minimum required version.

The example file and tutorial in the documentation need extra dependencies. Julia is not my main language so my environment is rather empty and I had to install a bunch of additional packages (Distributions, SimpleValueGraph, LoggingExtras, StatsBase, SimpleWeightedGraphs, MetaGraphs, Agents). I recommend to update the documentation accordingly in order to save back-and-forth operations to install dependencies 1 by 1. This also regards the readme file in the example folder. The latter could be renamed README.md in order to automatically render when the folder is opened (I leave that to the authors' choice).

I did not find guidance to run the tests, I simply ran runtests.jl but the procedure should be mentioned somewhere. The test coverage seems very decent, but node_aligned_edge_colored_graph.jl did not succeed and 23 were labelled as broken (maybe it I am not running the tests correctly though).

The documentation is thorough, complete and includes detailed API. However, as I am not very familiar with the specificity of multilayer graphs, I find the documentation a bit dense. While all the info is there, the tutorial remains quite abstract and theoretical. This this definitely not a requirement for publication, but I suggest the package would really benefit from a couple of simple applied use cases (i.e. beyond "node_1, layer_1, ...), with maybe some visualization. It would help potential users less familiar with the method and/or Julia(multilayers)graph.jl` ecosystem to use the tool for their research/production more rapidly.

pitmonticone commented 1 year ago

Dear @bgailleton,

Thank you very much for your precious review comments and suggestions.

Installation

Installation was straightforward, but I echo @TomKellyGenetics that there might be minor versioning conflicts. I managed to install it on Julia 1.8.6 (Ubuntu 22.04) but had to update Julia before. It is not a problem but the documentation should explicitly mention the minimum required version.

Usage Example

The example file and tutorial in the documentation need extra dependencies. Julia is not my main language so my environment is rather empty and I had to install a bunch of additional packages (Distributions, SimpleValueGraph, LoggingExtras, StatsBase, SimpleWeightedGraphs, MetaGraphs, Agents). I recommend to update the documentation accordingly in order to save back-and-forth operations to install dependencies 1 by 1. This also regards the readme file in the example folder. The latter could be renamed README.md in order to automatically render when the folder is opened (I leave that to the authors' choice).

Test

I did not find guidance to run the tests, I simply ran runtests.jl but the procedure should be mentioned somewhere.

To test any installed Julia package one needs to run the following code:

using Pkg
Pkg.test("Package")

Since this is usually considered common knowledge within the Julia community and we already adopts CI actions which run each time we push any commit to the main branch we would prefer not explicitly mentioning such a procedure in the README or in the documentation.

The test coverage seems very decent, but node_aligned_edge_colored_graph.jl did not succeed and 23 were labelled as broken (maybe it I am not running the tests correctly though).

Those broken tests are voluntarily broken so no changes are required there.

Conclusion

I've added an author checklist in the main thread to keep track of all the requested changes.

bgailleton commented 1 year ago

Thanks a lot for addressing these comments!