-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: PyEscape #2072
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @pdebuyl, @markgalassi it looks like you're currently assigned to review this paper 🎉. ⭐ 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:
|
|
@drvinceknight I can't tick the boxes in the review checklist. From memory, I was able to edit the checklist directly in my earlier work with JOSS. Is there anything that I should do to get the authorization? |
That's strange, I've just checked and I can tick them so I'd assume you can because I believe you have all the necessary authorisation. Could you double check and also perhaps try on a different browser? |
My bad @drvinceknight I needed to renew my invitation to the reviewers group. I thought that it would not be needed for reviewers having already served. |
No problem, glad it's sorted 👍 |
Hi @AoifeHughes , I had a first look at the program. I believe that documentation needs to be improved. I have FunctionalityInstallation: Does installation proceed as outlined in the documentation?
Functionality: Have the functional claims of the software been confirmed? I ran the Example notebook with success. I would like to have a least a few benchmark cases. DocumentationA statement of need: Do the authors clearly state what problems the software is designed to The problem is well stated. The target audience is not. Installation instructions: Is there a clearly-stated list of dependencies? Ideally these A requirements.txt file is missing. This is the most standard way to state dependencies for Example usage: Do the authors include examples of how to use the software (ideally to solve There is one example notebook. I suggest to improve the example by adding a textbook-type Functionality documentation: Is the core functionality of the software documented to a No. There are no docstring and no module-wide documentation page. The purpose of the Automated tests: Are there automated tests or manual steps described so that the There is limited testing. The test checks against an upper limit. An output time of zero Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute There is a contribution section in the readme. It only applies to new features and not to |
@pdebuyl Thank you for the detailed response, and for taking the time to review. I will address these issues and report back within the next week. |
I have just pushed a series of commits which I feel address most of the concerns: Functionality
Documentation
OtherI am happy to take on board any other issues that reviewers may have and will address them promptly also. Thank you again for your valuable input. |
I would also like to add, we are currently preparing to submit a paper which will cite this software, when it is published we will be able to provide additional real-world solutions. Until then, we are reluctant to add additional examples to prevent self-plagiarism or to remove novelty from our current research. |
@markgalassi apologies for the nudge, do you know when you might have a moment to carry out your review? |
@AoifeHughes the doi for the JOSS paper should already be known. @drvinceknight is it ok to cite a JOSS paper before its acceptance? |
I don't think it would be appropriate to cite before acceptance, the DOI currently is inactive it seems. |
Yes the DOI is not yet an actual representation of the software as it might be modified still. I'm still waiting to hear back from @markgalassi but if I don't soon I will look for another reviewer. |
I just got back from travel and dove in to the paper. I am afraid that it is clearly not a candidate for anything yet. The simplest of things (following the procedure in README.md) fails: gives:
Clearly some variable p should be set, but for a minimal example which is the first thing shown this is not given! Please have the author double-check that the README.md works and then we can get back to work on evaluating this software. |
Let me also add a comment on the idea of notebooks as documentation: I know they are much in vogue, but there are people (like me) who insist on reproducible procedures for running python code as batch. This means that I am not in the habit of loading notebooks, and if I am presented with documentation as a notebook I want a command line invocation which will load all that. So pointing to a directory is not enough: please give a command to load that notebook up from a standard GNU/Linux system. |
(sorry: clicked wrong button and "close"ed the issue; I think that this reopens it) |
Thanks for your comments. I have added the required lines to the read me. Re Notebooks: For the case of being documentation and providing an example of how to use the software, then the integrated github viewer should be enough. This is presented as a method without results (beyond solving a known problem), so I wouldn't expect running it to be a complete necessity. Saying that however, most users will want to run notebooks differently and depending on your current setup you should be able to run Though, if you're wanting that command-line experience then running
from bash (providing you installed the library) will give the same results, without plots as in the example notebook. |
Thank you for the update. I ticked many checks for the review already. Some items remain to
I filed pull requests for 7 and part of 6. |
Further testing: thanks @AoifeHughes for addressing the issues I reported previously. Smaller ones now:
So p and dt are not defined. This is the usual problem with the notebook people trying to write reproducible and deliverable software and documentation :-) Otherwise my earlier problems seem to have been addressed and I am continuing with the checklist. |
Again, thank you for the comments and suggestions. Particularly towards testing and the errors in the examples, I was running them sequentially and so variables were carried over. I've made updates to the readme to remove the unnecessary text also.
I've also updated several tests to use the numpy floating point closeness functions. Pull requests for minor fixes have been accepted. |
I have checked most of the boxes in the paper checklist. Here is a small suggested diff (below) so you don't assume that users are steeped in markdown. I would suggest that the "statement of need" in the readme.md file could do with a bit more, and that the paper put a math citation at the moment of stating "The mathematical models provided are simple and robust @iWouldCiteMySourceHere". I also have not yet checked the "state of the field" checkbox. In the paper you discuss the Schuss and Holcman papers, but no reference to existing software. You say yours is "novel" but you do not say that there is no "narrow escape" softare. And another nit: when I did a "git diff" after running your notebook procedure I saw a diff on a date in the notebook. Remember: notebooks are not reproducible and they are not entirely human source, so it is flawed to add them to a version control repo. I would not hold up the paper on this, but I would recommend that you put reproducibility and best VC practices front-and-center: commit a .py file, then provide a simple one-liner to load that into a notebook for notebook types. Other those simple suggestions I think you meet the criteria and I would quickly finish this off. And if I might add a personal note: your project is interesting! It made me want to read up on narrow escape and take an interest in the topic.
|
@AoifeHughes everything looks good to me. Could you make a tagged release and archive it (on Zenodo). When you've done that, report the version number and archive DOI here please. Please make sure the archive deposit has the correct metadata (title and author list) to match the paper. If you could also confirm the version number, it's currently |
Hi @drvinceknight, Version will be at 1.0 as to me this published version should be marked as that. Do let me know if I need to do anything more or if I've made any errors. Thanks! |
@whedon set 10.5281/zenodo.3725946 as archive |
OK. 10.5281/zenodo.3725946 is the archive. |
@whedon accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1400 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1400, then you can now move forward with accepting the submission by compiling again with the flag
|
Sorry for the delay @AoifeHughes, I've recommended acceptance. Thank you @pdebuyl and @markgalassi for your time and effort reviewing this work: it's really appreciated. |
Thanks - I'll work on finishing this |
@AoifeHughes - please update the archive's metadata so that the title matches the paper title |
Additionally, I've suggested some changes to the paper in https://github.com/AoifeHughes/NarrowEscapeSimulator/pull/6 |
@whedon generate pdf |
Hi @danielskatz, thank you for the changes, I've reviewed and merged the changes, and just updated the zenodo title. |
@whedon accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#1401 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#1401, then you can now move forward with accepting the submission by compiling again with the flag
|
@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... |
Thanks to @pdebuyl & @markgalassi for reviewing, and @drvinceknight for editing! And congratulations to @AoifeHughes and co-authors! |
🎉🎉🎉 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:
|
Submitting author: @AoifeHughes (Aoife Hughes)
Repository: https://github.com/AoifeHughes/NarrowEscapeSimulator
Version: 1.0
Editor: @drvinceknight
Reviewer: @pdebuyl, @markgalassi
Archive: 10.5281/zenodo.3725946
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
@pdebuyl & @markgalassi, 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 @drvinceknight know.
✨ Please try and complete your review in the next two weeks ✨
Review checklist for @pdebuyl
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @markgalassi
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: