-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
[REVIEW]: pyrexMD: Workflow-Orientated Python Package for Replica Exchange Molecular Dynamics #3325
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @janash, @rosecers it looks like you're currently assigned to review this paper 🎉. 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. ⭐ Important ⭐ 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
|
👋 @janash, please update us on how your review is going (this is an automated reminder). |
👋 @rosecers, please update us on how your review is going (this is an automated reminder). |
Hi @whedon - I've gone over the COI and general checks and installed the dependencies. Planning to closely review the software later this week and this weekend! |
My largest concern comes from the citability and need within the community -- there are no publications that mention "pyrexMD", and the publications of the two authors using REMD (3 as of 2020) have not yet been cited*. To this end, the authors need to comment on two things: the breadth of scope of this software (it appears quite narrow), and whether similar workflows would be supported by pre-existing software (particularly SSAGES).
The results contained in the paper summary are reproducible in the examples. Beyond that, no functional claims have been made.
The authors have an explicit statement of need in their write-up; however, mentioned above, they have not addressed the breadth of the scope of this package and the currently available software packages intended to this end.
The requirements for pyrexMD need to be more prominently featured. For example, some packages require specific python versions, which may not be clear upon installation.
The authors include examples, however, these are not well-documented and require more explanation / commenting, without which I can't know what the examples are intending to demonstrate or if the results are as expected. Additionally, the authors should embed some examples in the documentation -- if there is a user considering using this package, they may want to see a demonstration of the package before downloading and without searching through the API.
There is API documentation -- it is sufficient, but could greatly benefit from further formatting and editing.
The tests are sufficient, however, they currently raise several warnings, which should be remedied. As for coverage, no coverage test or CI is implemented to check on the functionality.
No.
The summary is minimal -- the authors should expand considerably on the REX methodology and the workflows supported by pyrexMD.
There is an explicit statement of need that needs to be amended (described in preceding comments).
No.
The authors are fairly terse in their writing, and need to expand in several places (e.g. the figure explanations and applications).
Overall, the examples, use cases, and need for this software need to be expanded upon. The examples should include commentary as to the expected behavior and "behind the scenes" operation of the software, including an introductory paragraph at the top of the example to explain what is being demonstrated. The authors have not identified use cases for this software beyond their own publications -- they should detail in their write-up examples of the sorts of analyses possible with this software. Finally, there needs to be some statement as to the novelty of this software -- is it fulfilling a need within the community which is not currently being met? |
thank you for the feedback. i will try to solve most of the things mentioned over this weekend. |
This software mainly seems to be a wrapper around three libraries - gromacs, nglview, and mdanalysis. The software has not yet been cited in academic papers. In its current state, the paper does not appear to meet the definition of substantial scholarly effort.
I had difficulty installing the software due to a missing dependency with mpi4py on my mac. I used
I feel that I need expanded example usage documentation to determine this. The main functional claims of the software are in the last paragraph of the statement of need. It might be beneficial to number the examples as tutorials and reference the tutorial numbers in the paper.
No performance claims made.
There is a list of installation instructions. I did encounter an error with dependencies as described above. I have a few comments about the installation instructions:
There is example usage, but the documentation associated with the example usage needs to be expanded. Please use a combination of markdown and code cells in your example notebook to give an explanation of tutorials.
Please move documentation link on the readme to top. There is API documentation that is adequate for using the package. However, it would benefit from additional documentation or better organization.
There is a set of tests which can be run with pytest (manual steps), but there are no automated tests.
No.
No. Please expand this portion.
Summary I also suggest automated building and testing of your package using GitHub Actions, and putting your package on PyPI for ease of installation. |
I'd like to echo @janash's difficulty with installation, as I also had issue with mpi4py on my mac; I concur that the authors should test installation on supported operating systems. Ultimately, I needed to run:
|
sorry for the late response. I did some CI with building and testing the software (took me longer than expected) and reworked the tutorials (juypter notebooks). Now ill expand the online documentation and then revisit the paper. |
Update about the tasks:
Todo:
|
A few notes:
|
@jgostick we pushed a new version of paper.md and extended the citations accordingly. I think all points mentioned from the reviewers should be covered now. |
@whedon generate pdf |
Thank you @ArtVoro, the documentation is much improved. I would still like to see some additional comments/explanation in the example notebook tutorials. I have concerns about code quality and maintainability. Particularly, a large amount of code is repeated when it need not be. Please see the issue I have filed on your repository (KIT-MBS/pyrexMD#1). I would suggest some sort of decorator for the gromacs functions instead of the current implementation. This is relevant for a few reasons: 1 - Repeating code in this way is a bad practice. Since the functionality is the same for all these parts, it will be harder to maintain or change in the future (think having to change several places vs one place if you want to change behavior). 2. JOSS lists lines of code as a metric in scholarly effort. |
thanks for the feedback. the repeated code on gmx.py was actually on my todo list but had low priority. however since its not much effort i can fix it over this weekend. about the notebooks, i can take another look at it and add more comments/explanation as you suggested but I dont know when exactly. Im working on 2 papers right now and my schedule is already packed. due to the delays in reviews I want to send out the drafts first. |
i fixed the repeated code by calling help functions now. |
Hey @ArtVoro, I've finally been able to get another look at this and was once again in a tornado of trying to install the package and run the tests. Comments on installation and testing:
Comments on what I've been able to get through in the code:
All in all the documentation, paper, and code have been improved, but still need to be streamlined. I'll try to give another look-over next week. |
OK, @jarvist is now a reviewer |
@janash, @rosecers I am not sure if you've followed the developments on this JOSS submission after you worked on reviews. In short, we have had another person contribute to the review and it has been found in good enough shape for acceptance. Currently you are both listed as reviewers and we'd like to list you as reviewers in recognition of your efforts here, but understand if you do not wish to be listed. Could you please comment here as to your preference either way? |
Thank you @janash! |
I emailed @rosecers about this in case she doesn't receive github notifications. Will let you know when I hear. |
@rosecers has also given her go-ahead to list her as a reviewer, so this is good-to-go |
@kthyng just pinging you, since I asume you don't get automatic notifications. |
Ok, great sounds like the reviewer question is answered fully! I'll go through the rest of the final steps. |
@ArtVoro things look good, though please preserve capitalization for Jupyter in a reference with {} in the .bib file around the word or character. Also check the other references carefully for correct capitalization. |
@kthyng i checked the reference titles based on the proof from openjournals/joss-papers#2780. I fixed some and they should all be correct now. |
@whedon generate pdf |
Ok looks good! |
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
I would also like to thank you all for your help with this review. Best regards |
Submitting author: @ArtVoro (Arthur Voronin)
Repository: https://github.com/KIT-MBS/pyREX
Version: v1.0
Editor: @jgostick
Reviewers: @janash, @rosecers, @jarvist
Archive: 10.5281/zenodo.5744760
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 badge code:
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
@janash & @rosecers, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jgostick 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 @janash
Conflict of interest
Code of Conduct
General checks
I believe that this check mark is what has been causing the delay for both me and @rosecers. Unfortunately, when this paper was first submitted, the package was not well-documented and was hard to understand. We have asked for some clarification and code clean up. Unfortunately, I feel that even with the improvements this work does not meet the requirements of JOSS for substantial scholarly effort and I can't recommend this package for publication in JOSS at this time.
JOSS defines the following for substantial scholarly effort:
As a rule of thumb, JOSS’ minimum allowable contribution should represent not less than three months of work for an individual. Some factors that may be considered by editors and reviewers when judging effort include:
I unfortunately cannot recommend this software package for publication in JOSS at this time. I think it would be more appropriate for the author to obtain a DOI through something like Zenodo and resubmit for publication if the package proves sufficiently useful.
Functionality
Documentation
Software paper
Review checklist for @rosecers
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: