search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
714 stars 38 forks source link

[REVIEW]: CliquePercolation: An R Package for conducting and visualizing results of the clique percolation network community detection algorithm #3210

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @LangeJens (Jens Lange) Repository: https://github.com/LangeJens/CliquePercolation Version: v0.3.0 Editor: @karthik Reviewer: @donaldRwilliams, @AlexChristensen Archive: 10.5281/zenodo.4844318

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/04144f9d6699c61e1f7205ec45779571"><img src="https://joss.theoj.org/papers/04144f9d6699c61e1f7205ec45779571/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/04144f9d6699c61e1f7205ec45779571/status.svg)](https://joss.theoj.org/papers/04144f9d6699c61e1f7205ec45779571)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@donaldRwilliams & @AlexChristensen, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @karthik know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @donaldRwilliams

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @AlexChristensen

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @donaldRwilliams, @AlexChristensen it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
karthik commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

Failed to discover a Statement of need section in paper

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.20 s (65.0 files/s, 17406.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                                7             95            913            734
XML                              1              0              8            612
TeX                              2             22              0            218
Rmd                              1            174            267            190
Markdown                         2             92              0            156
-------------------------------------------------------------------------------
SUM:                            13            383           1188           1910
-------------------------------------------------------------------------------

Statistical information for the repository 'e7dabacd87d8b5c208195611' was
gathered on 2021/04/22.
No commited files with the specified extensions were found.
karthik commented 3 years ago

Thank you @donaldRwilliams, @AlexChristensen for serving as reviewers. Please follow the checklist assigned to you above and ping me with any questions or issues that arise.

karthik commented 3 years ago

@LangeJens Please add a statement of need section to your paper.

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/bioinformatics/btl039 is OK
- 10.1038/nphys2188 is OK
- 10.1086/386272 is OK
- 10.18637/jss.v048.i04 is OK
- 10.1088/1367-2630/9/6/180 is OK
- 10.1016/j.physrep.2009.11.002 is OK
- 10.1038/nature03607 is OK

MISSING DOIs

- 10.31219/osf.io/muvc3 may be a valid DOI for title: Emotions as Overlapping Causal Networks of Emotion Components: Implications and Methodological Approaches

INVALID DOIs

- None
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

LangeJens commented 3 years ago

@karthik The Statement of Need is part of the first section as indicated by the heading.

karthik commented 3 years ago

@LangeJens Apologies for that. The bot looks for a separate heading but I think what you have is fine. We'll ignore this warning going forward.

AlexChristensen commented 3 years ago

@LangeJens and @karthik,

I've gone through the review checklist and see that everything is OK.

I ran through the example in the manuscript with some of my own data and made a few updates to the package's code. Here are the changes that I made in my fork: https://github.com/AlexChristensen/CliquePercolation

Alex's changes for JOSS review

o For cpAlgorithm, cpPermuteEntropy, and cpThreshold, I made it so that any matrix can be input (rather than just qgraph objects). The code still does not specify for a square or symmetric matrix, which would be recommended. Having input as either a matrix (data.frame) or qgraph object allows your users greater flexibility

o Specific for cpPermuteEntropy: I added code to parallelize the function to increase the computational speed. Although the old code worked well for a small number of variables, I performed the analysis on one of my own datasets with 48 variables and this took 82.16 seconds. With the parallelization, the same analysis took 22.25 seconds (or about four times faster). The parallelization should scale with the sample size. Here's my computer specs: Windows 10 (x64), Intel i7-9750H 2.60GHz, 32GB RAM

o Note that I changed the DESCRIPTION file to add the parallel package as an IMPORT for the parallelization

o I also changed the .Rd of cpPermuteEntropy to add the ncores argument. The default is for half the processing cores on the user's computer

@LangeJens, I can create a pull request if these changes are acceptable. Feel free to change the argument name and documentation for the ncores argument as necessary. I've commented out the original code to retain previous versioning. I've also tested the changes on a number of datasets to make sure that my additions did not introduce any errors.

LangeJens commented 3 years ago

@AlexChristensen and @karthik,

Thank you very much Alex. I am very grateful that you took the time to improve my code. Especially the implementation of parallel computing is amazing! I would be happy if you could create a pull request that I can then merge with the master branch. After merging, I will add a few other changes:

In case you disagree with any of the changes, please let me know.

LangeJens commented 3 years ago

I merged the pull request, implemented all the changes I listed above, and pushed the changes to the master branch. I also updated the reference in the paper that did not yet have a doi assigned when I submitted.

@AlexChristensen I noticed during testing that setting a seed with set.seed() during parallel computing does not work. I could not find an easy fix online. Do you have a recommendation? Your help would be highly appreciated.

whedon commented 3 years ago

:wave: @donaldRwilliams, please update us on how your review is going (this is an automated reminder).

donaldRwilliams commented 3 years ago

@karthik I will do this review over the weekend. Apologies on the delay.

AlexChristensen commented 3 years ago

@LangeJens,

I've added code that will allow for reproducible parallelization (see latest pull request). This was done using the clusterSetRNGStream function in the parallel package.

With this, I added an argument called seed that defaults to NULL. The default will produce stochastic results. To get reproducible results, you can set the seed argument.

Like the example in the paper, you can use:

thresholds.permute <- cpPermutateEntropy(W, cpThreshold.object = threshold, ncores = 2, seed = 4186)

You would no longer need to use set.seed unless you were wanting to set the seed for the rest of the functions.

donaldRwilliams commented 3 years ago

@LangeJens Fell a bit behind, but have not forgotten about this. apologies on the delay

LangeJens commented 3 years ago

@AlexChristensen Thank you very much! I merged the pull request and tested the function. Everything works perfectly. @donaldRwilliams Absolutely no problem!

donaldRwilliams commented 3 years ago

@karthik It wont let me edit the checklist.

I get the following error

image

donaldRwilliams commented 3 years ago

@karthik I have opened up 8 issues on the repo. I think the proposed changes will improve the package considerably.

  1. Currently, the package contains no real world examples. (https://github.com/LangeJens/CliquePercolation/issues/3). Rather the examples are "fake" networks when it would be ideal to have at least one example with real data, I think.
  2. Related to 1., I suggest adding at least one dataset to the package (https://github.com/LangeJens/CliquePercolation/issues/4)
  3. The README is currently quite bare. It could include one example. As it is, the user is faced with a rather large block of text (a paragraph) but no example. Also, for things the package does do, the paragraph can be broken up into bullet points (https://github.com/LangeJens/CliquePercolation/issues/5)
  4. Although the package is small, I suggest adding some kind of continuous integration (https://github.com/LangeJens/CliquePercolation/issues/6)
  5. Add CRAN, download, JOSS paper, and CI badges to README (https://github.com/LangeJens/CliquePercolation/issues/7).
  6. Currently, the package seems to only support qgraph objects, which is on the user to deal with, when this can be handled internally in the package (https://github.com/LangeJens/CliquePercolation/issues/8).
  7. The package does not have any summary or print functions, which means it just outputs at times a rather unruly list (https://github.com/LangeJens/CliquePercolation/issues/9). I provided one example of a print function in that issue.
  8. Related to 7, there could be functions added to extract the information, e.g., for the communities (https://github.com/LangeJens/CliquePercolation/issues/11). Rather than just accessing the object in a list (which is less than ideal).
donaldRwilliams commented 3 years ago

@karthik @LangeJens Note the above suggestions on the one hand are "cosmetic", but on the other, I find that nicely developed software often include all the above (even software on the smaller side). I am confident that making those changes will greatly improve the users experience and potentially increase impact (e.g., honing in the README, real world examples, etc.).

Here are examples for my smaller packages:

https://github.com/donaldRwilliams/IRCcheck

https://github.com/donaldRwilliams/vICC

https://github.com/donaldRwilliams/GGMncv

danielskatz commented 3 years ago

@Whedon re-invite @donaldRwilliams as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@donaldrwilliams please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

LangeJens commented 3 years ago

@donaldRwilliams Thank you very much for taking the time to provide all these recommendations! I will do my best to implement them asap.

LangeJens commented 3 years ago

@donaldRwilliams I addressed all issues in the repository. I again want to thank you for pushing me to implement these changes. As many of these things were entirely new to me, I hope you are nevertheless satisfied with how I implemented them. I also edited the vignette to cover the new functionality.

@karthik I also adapted the paper to the new changes.

donaldRwilliams commented 3 years ago

@LangeJens Wow !

I just took a look at the README and it is very (very) impressive.

Also note I suggested the changes in an effort to (hopefully) increase the impact of the package. I like it all a lot :-)

donaldRwilliams commented 3 years ago

@karthik I just finished the review.

LangeJens commented 3 years ago

@donaldRwilliams Thank you very much for your kind words! I agree that the changes you proposed will make the package more useful, accessible, and fitting to the classic approach in R. I always thought that the vignette is most helpful (and for some people it was), but I guess many people won't know about how to find it.

LangeJens commented 3 years ago

@karthik Is there anything else I can do?

karthik commented 3 years ago

@whedon check references

karthik commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/bioinformatics/btl039 is OK
- 10.1038/nphys2188 is OK
- 10.1086/386272 is OK
- 10.18637/jss.v048.i04 is OK
- 10.1088/1367-2630/9/6/180 is OK
- 10.1016/j.physrep.2009.11.002 is OK
- 10.1177/1754073920988787 is OK
- 10.1038/nature03607 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

karthik commented 3 years ago

@whedon accept

whedon commented 3 years ago

No archive DOI set. Exiting...

karthik commented 3 years ago

@LangeJens Everything looks good. Please deposit the package in Zenodo and then post the DOI here. Also please confirm that the version of the package is now 0.3.0

LangeJens commented 3 years ago

@karthik I deposited the package in Zenodo. The doi is 10.5281/zenodo.4844318. I also submitted the package as version 0.3.0 to CRAN and they confirmed that it is soon online.

karthik commented 3 years ago

@whedon set v0.3.0 as version

whedon commented 3 years ago

OK. v0.3.0 is the version.

LangeJens commented 3 years ago

Should I set the archive?

karthik commented 3 years ago

@whedon set 10.5281/zenodo.4844318 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4844318 is the archive.

karthik commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/bioinformatics/btl039 is OK
- 10.1038/nphys2188 is OK
- 10.1086/386272 is OK
- 10.18637/jss.v048.i04 is OK
- 10.1088/1367-2630/9/6/180 is OK
- 10.1016/j.physrep.2009.11.002 is OK
- 10.1177/1754073920988787 is OK
- 10.1038/nature03607 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2351

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2351, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
LangeJens commented 3 years ago

To me, the manuscript looks great. However, I cannot evaluate the correctness of the Crossref deposit XML.

I again want to thank @donaldRwilliams and @AlexChristensen for their reviews and the effort they invested! Thanks also to @karthik for overviewing the process and especially for trying hard to find these great reviewers in the first place!

karthik commented 3 years ago

@LangeJens that crossref message is for the associate editors. Please ignore.

An AEIC will be here shortly to check everything over before accepting the paper. Thanks again for your submission and patience.

Kevin-Mattheus-Moerman commented 3 years ago

@LangeJens I'll be helping to process acceptance and publication of this work. I have check the archive link and have proofread your paper. Below are some remaining issues that need your attention: