search

openjournals / joss-reviews

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

[REVIEW]: IGraph/M: graph theory and network analysis for Mathematica #4899

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@szhorvat<!--end-author-handle-- (Szabolcs Horvát) Repository: https://github.com/szhorvat/IGraphM Branch with paper.md (empty if default branch): joss-paper Version: 0.6.5 Editor: !--editor-->@prashjha<!--end-editor-- Reviewers: @leotrs, @pitsianis Archive: 10.5281/zenodo.7469228

Status

status

Status badge code:

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

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

@leotrs & @pitsianis, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @prashjha 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 ✨

Checklists

📝 Checklist for @pitsianis

📝 Checklist for @leotrs

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.36 s (237.9 files/s, 116312.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Mathematica                     51           5379           1882          15584
XML                              1             19              5           6675
C/C++ Header                    10           1369           1127           5552
C++                              3            328            162           1099
CSS                              1            208            180            698
Markdown                        10            346              0            677
TeX                              1             11              0            106
Lua                              1              9              9             82
YAML                             2              1              4             22
JSON                             1              0              0             18
Bourne Shell                     4              2              0              8
-------------------------------------------------------------------------------
SUM:                            85           7672           3369          30521
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1238

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/CBO9781139164849 is OK
- 10.5281/zenodo.3630268 is OK
- 10.1016/j.entcs.2011.06.003 is OK
- 10.1088/2632-072X/abced5 is OK
- 10.1016/j.dam.2006.07.016 is OK
- 10.1016/0031-3203(80)90066-7 is OK
- 10.1016/B978-0-444-87806-9.50013-X is OK

MISSING DOIs

- None

INVALID DOIs

- None
prashjha commented 1 year ago

Dear, @leotrs and @pitsianis, please read the first couple of comments in this thread and create your review checklist. You can read the reviewer guidelines here. Also, you can browse the closed "REVIEW" issues on the "joss-reviews" repository to get some ideas on how to complete the reviews. Good luck!

editorialbot commented 1 year ago

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

pitsianis commented 1 year ago

Review checklist for @pitsianis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

leotrs commented 1 year ago

Review checklist for @leotrs

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

leotrs commented 1 year ago

Hello @szhorvat just noting here that I tried to run your software on the Wolfram Cloud and it won't run. The installer prints a nice error message in this case, which is great! So thanks for that. The reason seems to be that a necessary dependency called LibraryLink is not supported on Wolfram Cloud.

Is LibraryLink 100% needed? If your software was able to run on Wolfram Cloud, which doesn't require buying a license from Wolfram Research, it would greatly increase the audience of your package. If this is completely necessary, then I understand, I just thought I'd ask.

As an aside, my review will be delayed a couple of days as I get my hands on a Mathematica license so I can test the software.

szhorvat commented 1 year ago

The bulk of the library is written in C and C++, which allows it to have good performance, and which is necessary for interfacing with several dependencies. Wolfram Cloud disables not just LibraryLink, but all means of running arbitrary C code (I assume for security reasons), so unfortunately there is no way to make the package compatible with the Wolfram Cloud. I am hoping that Wolfram will change this at some point in the future.

To get this out of the way:

There is the "Free Wolfram Engine", which you can install on your own machine. IGraph/M does work with this. However, the Free Wolfram Engine does not include the graphical notebook interface. I think this changes the user experience to such a degree that a proper review is not possible: It won't be possible to run documentation examples directly, there won't be auto-completion (the package has long function names, so this is essential) and none of the Dynamic functionality will be accessible. Therefore I would like to ask reviewers not to base their assessment solely on working with the Free Wolfram Engine either.

Just in case you have a Raspberry Pi, for which Mathematica is free, the paclet already includes binaries for that platform.

leotrs commented 1 year ago

This makes perfect sense, thank you! I would suggest that you add a note regarding these matters to your package's README. Specifically, I'd suggest mentioning that your package does not work in the Wolfram Cloud, and that the Wolfram Engine works but is not recommended.

szhorvat commented 1 year ago

I added a note in the readme about the Wolfram Cloud.

As for the Free Wolfram Engine, my comment above referred specifically to reviewing. Using the Free Wolfram Engine necessitates using the command line interface instead of the notebook interface. This makes for a frustrating experience, and does not represent how the vast majority of people use Mathematica. Therefore, it should not be the sole basis of reviews. This comment would apply equally to any other package, not just IGraph/M.

The same applies to using Jupyter with the Free Wolfram Engine, see https://mathematica.stackexchange.com/q/274333/12

That said, I do make sure that IGraph/M works well with the Free Wolfram Engine, and do test it occasionally. Command line usage does have its place in special circumstances (e.g. I use IGraph/M that way on our HPC cluster). You are welcome to try it out.

szhorvat commented 1 year ago

For the purpose of organizing the reviews better, you are welcome to open issues in the issue tracker even if they are not about a bug or feature request. The advantage compared to the forum (which we normally recommend) is that you can include a backlink to this JOSS issue, which will show up here.

leotrs commented 1 year ago

I was able to install and use it with ease. I have a soft spot for Mathematica AND graph theory, so I love this one. Thanks @szhorvat. I believe the paper can be accepted as is.

szhorvat commented 1 year ago

Thank you for the kind words and for taking the time to do the review @leotrs. Feel free to submit any feedback regarding the package anytime, even independently of the review.

szhorvat commented 1 year ago

I noticed that @pitsianis has checked all the review checkboxes. Thank you for taking a look at the package! However, since there was no verbal feedback, I would like to clarify whether this constitutes a completion of the review.

Due to recent changes in my circumstances, I need need to freeze work on this project for a while starting early next year. If at all possible, it would be very helpful for me to have an estimate of the review timeline, so I can plan accordingly.

pitsianis commented 1 year ago

I believe the reviewers are done. Great work. I implemented a wrapper of Leiden for Julia recently. Leiden makes good use of IGraph.

szhorvat commented 1 year ago

Thanks so much for taking the time to do the review @pitsianis! Should you have any feedback in the future, even independently of the review, feel free to let me know.

szhorvat commented 1 year ago

@prashjha Please let me know if anything else is expected of me now that the reviews seem to be complete.

Due to recent changes in my circumstances, I need need to freeze work on this project for a while starting early next year.

For the reason above, it would be useful for me to know if the paper can be finalized this year.

prashjha commented 1 year ago

Hi @szhorvat, I will review the draft today and give some minor feedback. If all is good, I will send the decision to the Editor in Chief for final approval by tomorrow.

Dear @leotrs and @pitsianis, thank you for your effort in reviewing this submission. I sincerely appreciate you both making the time.

szhorvat commented 1 year ago

Thanks so much @prashjha!

I'm also planning to issue a new minor release soon. It probably makes sense to synchronize this release with the publishing of the paper, so I'll wait for feedback before going ahead.

prashjha commented 1 year ago

Hi @szhorvat, if you like you can update the version. We can then link the updated version with your joss submission.

prashjha commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

szhorvat commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

szhorvat commented 1 year ago

New version is out now.

szhorvat commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

szhorvat commented 1 year ago

I needed to release another version to help deal with an issue I suspect exits in the newly released Mathematica 13.2.0 on Apple Silicon. Let's link the paper to 0.6.5, which is out now.

szhorvat commented 1 year ago

@prashjha Happy new year! Hope you had relaxing holidays. Let me know if anything else is needed from my side here.

prashjha commented 1 year ago

Hi @szhorvat, can you take care of these minor typos in your JOSS draft?

  1. Line 76. Reference to the section is missing in The output is ....
  2. Lines 78 to 97. Maybe using bullets for these examples will help. Also, Line 97 is probably incomplete. Please check.

Once you take care of these minor suggestions, I will hand your paper to EiC for final decision. Sorry for the delay.

prashjha commented 1 year ago

@editorialbot set 0.6.5 as version

editorialbot commented 1 year ago

Done! version is now 0.6.5

prashjha commented 1 year ago

Also, @szhorvat, can you please do (if not done already) a 'tagged' release of your code and archive the release using zenodo or other methods? Make sure that the zenodo archive's title matches this JOSS submission's title.

prashjha commented 1 year ago

@editorialbot check references

prashjha commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/CBO9781139164849 is OK
- 10.5281/zenodo.3630268 is OK
- 10.1016/j.entcs.2011.06.003 is OK
- 10.1088/2632-072X/abced5 is OK
- 10.1016/j.dam.2006.07.016 is OK
- 10.1016/0031-3203(80)90066-7 is OK
- 10.1016/B978-0-444-87806-9.50013-X is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

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

szhorvat commented 1 year ago

Also, @szhorvat, can you please do (if not done already) a 'tagged' release of your code and archive the release using zenodo or other methods? Make sure that the zenodo archive's title matches this JOSS submission's title.

@prashjha Can you please clarify what you need archived on Zenodo? The branch that contains the paper? Or the software itself? The Zenodo master DOI for the software is here: https://doi.org/10.5281/zenodo.1134932 The title is the software's name, not the paper's title. In order to change this, believe I need to create an entirely new release, as Zenodo does not usually allow modifying existing submissions. I wouldn't want to do that more than once, so I want to be sure I understand what you're asking.

prashjha commented 1 year ago

HI @szhorvat, yes, zenodo for software. You already have the zenodo archive, so you are good. One minor thing though: change the title of the zenodo archive to match the title of your JOSS paper.

Also, please incorporate my suggestions in the draft.

Let me know when both tasks are done. I will hand it over to the EiC for a final decision.

szhorvat commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

szhorvat commented 1 year ago

Hello @prashjha,

Also, @szhorvat, can you please do (if not done already) a 'tagged' release of your code and archive the release using zenodo or other methods? Make sure that the zenodo archive's title matches this JOSS submission's title.

This is now done. I was under the impression that Zenodo submissions were not editable, but it turns out that their metadata can still be changed.

  • Line 76. Reference to the section is missing in The output is ....

This was meant to be a reference to a figure, which got accidentally removed at the very beginning of the submission process. Sorry about that. It is now fixed.

  • Lines 78 to 97. Maybe using bullets for these examples will help. Also, Line 97 is probably incomplete. Please check.

Unfortunately, using bullet points would push the code into the right margin, or require a reformatting which is not helpful for readability (see screenshot below). Therefore, I would prefer to keep it as it is. Let me know if this is acceptable. If not, I'll make this change as well.

image
prashjha commented 1 year ago

Hi @szhorvat, no worries. I am okay with the current version. Thanks.

prashjha commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago

Paper is not ready for acceptance yet, the archive is missing

prashjha commented 1 year ago

@editorialbot set 10.5281/zenodo.7469228 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7469228

prashjha commented 1 year ago

@editorialbot recommend-accept