openjournals / joss-reviews

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

[REVIEW]: HyperNetX: A Python package for modeling complex network data as hypergraphs #6016

Closed editorialbot closed 4 months ago

editorialbot commented 9 months ago

Submitting author: !--author-handle-->@brendapraggastis<!--end-author-handle-- (Brenda Praggastis) Repository: https://github.com/pnnl/HyperNetX Branch with paper.md (empty if default branch): paper Version: v2.2.0p Editor: !--editor-->@danielskatz<!--end-editor-- Reviewers: @szhorvat, @IvanIsCoding, @drj11 Archive: 10.5281/zenodo.10795225

Status

status

Status badge code:

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

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

@szhorvat & @IvanIsCoding, 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 @danielskatz 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 @szhorvat

📝 Checklist for @drj11

📝 Checklist for @IvanIsCoding

danielskatz commented 5 months ago

@brendapraggastis & @bonicim - How are your updates coming?

bonicim commented 5 months ago

danielskatz

Updates are almost done; we expect to have them pushed to master fairly soon. Will post an update ASAP.

bonicim commented 5 months ago

@danielskatz Updates pushed to master branch and reflected in latest release, 2.1.4. PyPi has also been updated.

Additional comments on other hypergraph libraries are forthcoming.

danielskatz commented 5 months ago

@bonicim - When you are done with the changes, please use the command @editorialbot generate pdf to make a new PDF. editorialbot commands need to be the first entry in a new comment.

brendapraggastis commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

:warning: An error happened when generating the pdf. Paper file not found.

bonicim commented 5 months ago

I will look into the pdf generation error. I have an idea of what happened; will rerun the command once I've updated the paper branch.

bonicim commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

bonicim commented 5 months ago

@editorialbot generate pdf

Added section on comparison with other hypergraph libraries.

editorialbot commented 5 months ago

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

szhorvat commented 5 months ago

Let me know when you feel it's ready.

EDIT: Can you please make sure the references are added before the fullstop within the section about other hypergraph libraries?

danielskatz commented 5 months ago

@bonicim - did you see the note above ☝️ from @szhorvat? Can you respond please? (We're really close to done now)

brendapraggastis commented 5 months ago

@szhorvat Yes, I am looking at our latest copy. I think it is just a problem with the markdown used. Will let you know when it is fixed.

bonicim commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

brendapraggastis commented 5 months ago

@szhorvat and @danielskatz the latest version has references to the three most contemporary libraries to HNX that we know of along with their references. Hopefully this version is acceptable for publication.

danielskatz commented 5 months ago

@szhorvat - when you get a chance, please let us know if you agree with ☝️

szhorvat commented 5 months ago

I was still waiting for a response on a minor comment here: https://github.com/pnnl/HyperNetX/issues/143#issuecomment-1949265449

Perhaps other reviewers can comment on whether what is suggested (i.e. install everything in the conda base environment) is considered good practice. Python technical details are not my area of expertise.

My guiding principle with these comments was to try to make sure that academic users, many of whom might not be very technical, will be well-served by the documentation.

danielskatz commented 5 months ago

@IvanIsCoding, @drj11 - any thoughts on this? See the issue in the comment above for the discussion

bonicim commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

szhorvat commented 5 months ago

@brendapraggastis When citing igraph in the paper, could you please spell it with a lowercase "i" and also include https://arxiv.org/abs/2311.10260 (which will eventually become the main citation point, replacing the old DOI-less paper)?

Could you please correct the uppercase "i" to lowercase before finalizing? I'll write up my report soon.

szhorvat commented 5 months ago

This is my final report. Please note that I'll be away, and unresponsive, during the following week.

HNX is an ambitious package that is breaking new ground within a rapidly developing field. I am happy to see such packages published in JOSS, and I am looking forward to seeing further HNX development.

Some suggestions to the authors:

Openness of development HNX is open-source in the sense that it has an open source license, and that it is technically accepting contributions on GitHub. However, the development process is opaque, and does not project an image that encourages contributions. Looking at the list of PRs, the majority of non-trivial ones were closed with a comment that they were handled internally. There are very few truly external contributions, and those that exist did not leave a mark on the git commit history (i.e. the name of the actual contributor wasn't recorded). I think HNX could truly benefit from transitioning to more welcoming practices. Consider making development more open and especially crediting contributors in some way.

HNX widget The HNX widget is featured prominently in the paper, but it turned out to function only under very specific circumstances. The authors improved the documentation and stated that the widget is still in beta and under active development. However, there hasn't been any public development (or release) in 3 years. Is this another symptom of the behind-closed-doors development practices? I am hoping to see significant improvements in the near future.

Documentation I found the documentation to be a bit terse for a scientific package (see e.g. https://github.com/pnnl/HyperNetX/issues/141) I know that keeping documentation minimal is very common with Python packages—HNX is not different in this respect. But give the target audience (researchers) and the mathematically non-trivial topics the package deals with, I think that putting more effort into documentation would be one of the biggest improvements HNX could make at the moment. Initiatives like pyOpenSci are already pushing for better documentation practices, I suggest taking a look at their guidelines, or just looking at common practices used in the communities of more scientifically oriented languages (e.g. R or Mathematica).

@bonicim @brendapraggastis Thank you for the patience. I hand over to @danielskatz now.

IvanIsCoding commented 5 months ago

@IvanIsCoding, @drj11 - any thoughts on this? See the issue in the comment above for the discussion

For the record on the Jupyter Widget debate.

With regards to packaging, ideally users would only pip install hnxwidget and just use it. But I myself have experienced issues with Jupyter Widgets as a user so I imagine that on the packaging side it can be challenging as well. A solution using both pip and conda is not ideal, so my suggestion is to package hnxwidget for Conda.

@brendapraggastis When citing igraph in the paper, could you please spell it with a lowercase "i" and also include https://arxiv.org/abs/2311.10260 (which will eventually become the main citation point, replacing the old DOI-less paper)?

Could you please correct the uppercase "i" to lowercase before finalizing? I'll write up my report soon.

I think this is one is on the journal, the template capitalizes everyword. The author's bibtex is what you would expect.. I think if they switch to use {i}graph in Bibtex it will preserve the capitalization?

szhorvat commented 5 months ago

I think if they switch to use {i}graph in Bibtex it will preserve the capitalization?

Yes, that works, but I meant to refer to the mention in the main text :-)

image

Also, to make sure there is no confusion, I do recommend publishing.

danielskatz commented 5 months ago

@IvanIsCoding - having now had all the reviews finish, we can go ahead and publish. Let me know if you want to make and changes at this point based on @szhorvat's comments. Once you are ready, I'll proofread the paper, and ask you to perform some archiving steps, then I'll finish the process.

IvanIsCoding commented 5 months ago

@IvanIsCoding - having now had all the reviews finish, we can go ahead and publish. Let me know if you want to make and changes at this point based on comments. Once you are ready, I'll proofread the paper, and ask you to perform some archiving steps, then I'll finish the process.

I have no objections for the software. For the paper, my only objections are minor igraph citation details. I agree with the other reviewer on the spelling, it is an easy change to make. Another factor that I agree is that igraph has recently posted https://arxiv.org/abs/2311.10260, so I do agree with @szhorvat's comment to cite the pre-print and incorporate the decade of work spent improving the package.

danielskatz commented 5 months ago

sorry @IvanIsCoding - I meant to address this comment to @brendapraggastis

Let me know if you want to make and changes at this point based on @szhorvat's comments. Once you are ready, I'll proofread the paper, and ask you to perform some archiving steps, then I'll finish the process.

brendapraggastis commented 5 months ago

@danielskatz @IvanIsCoding @szhorvat Thanks for catching the Igraph error. We will fix that and the references this week and notify you as soon as the final is ready.

bonicim commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

bonicim commented 5 months ago

@danielskatz @IvanIsCoding @szhorvat igraph typo is fixed and igraph reference added.

brendapraggastis commented 5 months ago

@danielskatz @IvanIsCoding @szhorvat igraph typo is fixed and igraph reference added.

I believe we are good to go for a final proof. The footnote references will be updated by JOSS, correct?

danielskatz commented 5 months ago

yes, and thanks. I'll do the proof-reading a little later today

danielskatz commented 5 months ago

I've now suggested a bunch of minor changes in https://github.com/pnnl/HyperNetX/pull/150 - please merge this, or let me know what you disagree with, then we can continue the process

brendapraggastis commented 5 months ago

@danielskatz The typo fix went in the wrong place. a95c8c8 put it in a sequence of terms, which I don't think is what you meant. I went through and made sure the punctuation after the citations were correct and in the correct place. @bonicim will push the fix this morning.

danielskatz commented 5 months ago

Sorry about that and thanks for fixing it.

bonicim commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

danielskatz commented 5 months ago

@brendapraggastis - can you please check to make sure this is ok now?

brendapraggastis commented 5 months ago

@brendapraggastis - can you please check to make sure this is ok now?

@danielskatz The punctuation changes are reflected in the latest document. I don't see anything else, so hopefully we are good to go now.

danielskatz commented 5 months ago

Thanks. The next steps are for you to:

I can then move forward with accepting the submission.

bonicim commented 5 months ago

@danielskatz I have created a tagged release, v2.2.0p. The rest of the required steps are currently in progress.

danielskatz commented 5 months ago

@editorialbot set v2.2.0p as version

editorialbot commented 5 months ago

Done! version is now v2.2.0p

bonicim commented 5 months ago

Software has been stored on Zenodo at https://zenodo.org/records/10790797

DOI: 10.5281/zenodo.10790797

danielskatz commented 5 months ago

@editorialbot set 10.5281/zenodo.10790797 as archive

editorialbot commented 5 months ago

Done! archive is now 10.5281/zenodo.10790797

danielskatz commented 5 months ago

@editorialbot recommend-accept