Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @timClicks, @linuxscout 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:

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

2. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/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

Attempting PDF compilation. Reticulating splines etc...

👉 Check article proof 📄 👈

👋 @timClicks, @linuxscout - your job as a reviewer is to examine the paper and software repo, and to check off items in your review list as possible. When you have some issue where improvement is needed, either flag it here or with a new issue in the repository (mentioning this review issue). If you have any questions or problems, please let me know. (FYI, I'm mostly off-line this week and next and will be slower to respond than normal.)

Thanks @danielskatz. I'm also away until the middle of next week. I will aim to have my review completed by next Friday.

I work on review

👋 @linuxscout - thanks for your work so far. Are there issues that are preventing you from checking the remaining boxes? If so, please flag them here or with new issues in the repository (mentioning this review issue).

@whedon remind @timClicks in 6 days

Reminder set for @timClicks in 6 days

I work on testing the software.
For the automated tests, there is none in the repository.

No automated tests
lisc-tools/lisc#22

As we say in the review criteria

Authors are strongly encouraged to include an automated test suite covering the core functionality of their software.

• Good: An automated test suite hooked up to an external service such as Travis-CI or similar
• OK: Documented manual steps that can be followed to objectively check the expected functionality of the software (e.g. a sample input file to assert behavior)
• Bad (not acceptable): No way for you the reviewer to objectively assess whether the software works

So we do not require automated tests (though we encourage them), but we do require some tests.

In this case, if there are manual tests that you think are sufficient "to objectively check the expected functionality of the software (e.g. a sample input file to assert behavior)", that's enough for this criterion to be checked.

I think that tutorials are good as tests.

There are tests! In the folder lisc/tests is a set of unit-tests. So, from the repository home, they are in lisc/tests.

They are, I would say often smoke-tests, in that there is not always strong accuracy checking, but there are at least sanity check tests.

They are written with pytest. If you want to run them locally, then, with pytest installed, from inside the lisc code folder (so in lisc/lisc) run:
pytest .

I realize now it's not really documented anywhere how to run the tests. Is there a convention on how / where to note tests and how to run them?

Tests are also integrated with travis:
https://travis-ci.org/lisc-tools/lisc

You might add this info to the README, but certainly should add it to https://lisc-tools.github.io/lisc/, perhaps after installation (after you install, you test to make sure things are working...)

What I should do now?

What I should do now?

Nothing - your part is done, and thanks!

We're now waiting for @timClicks to do his review, check off his items, bring up needed changes, etc. He was away, but said he would have his review done around now.

I think we're also waiting for @TomDonoghue to add something about how to run the tests to the documentation.

👋 @timClicks, please update us on how your review is going.

@danielskatz My recommendation: accept, subject to the test amendments that have been mentioned already :)

I've worked through the tutorials, and think that this a very handy package.

In general, the API documentation is in an OK state, rather than a Good state (as per the review criteria). That reflects the relative immaturity of the project. lisc is in a much better state than other packages that are released at a 0.1 state.

As a side comment, the tests are written using a very common test framework for Python applications (pytest). However, this isn't included as a dependency in the requirements.txt file so installing the test framework manually is required.

@TomDonoghue thanks for taking the time to submit this to JOSS.

@danielskatz @linuxscout @TomDonoghue Sincerely apologise for delaying this process.

👋 @TomDonoghue - so it seem like the only remaining action for you is to add something about how to run the tests to the documentation.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

👉 Check article proof 📄 👈

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

👉 Check article proof 📄 👈

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

👉 Check article proof 📄 👈

Test Coverage

In terms of noting the test coverage, what I have done is add a section to the README, which gets built as the homepage of the documentation page, which mentions the tests, and how to run them, and the test dependency of pytest.

I'm actually not too sure what is considered convention / best practice for documenting tests & test coverage - so please let me know if this isn't the best answer! I haven't, for example, typically listed 'pytest' in the requirements.txt (it seems not necessarily standard to do so?). Would best practice be to list pytest in requirements, even if its not needed to use the package? Pytest is noted in setup.py. And/or, more generally, if anyone has any suggestions on additional or different ways to mention tests, please let me know!

API Documentation
I totally agree that the level of API documentation is middle ground, with it wanting for more examples with inputs / outputs, etc. That's definitely on the list of things to keep updating (lisc-tools/lisc#20).

Paper Notes
There weren't any specific comments on the paper itself. I did a last sweep through, with some very minor edits for wording, layout, and minor ref fixes. I rebuilt the paper above. I hope it's okay to do some last tweaks - just wanted to make nothing was weird there before it gets locked in. Let me know if there are any comments on the wording, etc - happy to tweak / fix up!

Thanks!

A huge thank you to @linuxscout & @timClicks for doing the review, and @danielskatz for managing! Please let me know if there is anything else I should do!

👋 @linuxscout & @timClicks - Please let me know your comments on the changes described above

@danielskatz LGTM. I've reviewed the new paper and the new README. Both look fine.

👋 @linuxscout - Please let me know your comments on the changes described above

👋 @linuxscout - Please let me know your comments on the changes described above
(last chance, then I'll assume this is ok with you based on your previous comments and your checking of all the items on your list)

Hi,
I think it okay for updates.
Thanks.

👋 @TomDonoghue - to finish up now, I need you to

1. deposit the repository in an archive (e.g. zenodo) and to report the DOI here
2. tell me the latest version number here
3. confirm that the paper text and references are final

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

👉 Check article proof 📄 👈

@whedon check references

Attempting to check references...

OK DOIs

- 10.1145/2623330.2623667 is OK
- 10.1145/3097983.3098057 is OK
- 10.1016/j.jneumeth.2012.04.019 is OK

MISSING DOIs

- None

INVALID DOIs

- None