JOSS Review Comments
Closed this issue · 11 comments
Hello, and congrats on your JOSS submission! Overall, the package and the paper are in a very good state, and I've ticked most of the items on my reviewer checklist already.
I have a few minor comments that shouldn't take too long to address. Please let me know if these make sense, I'm happy to discuss them further if needed:
The Paper
- 1. No code examples are given in the paper. Might it be nice to include some of the examples from the
README.mdfile? (See comments below, feel free to ignore this point or just include a link to the examples in the paper, to avoid code possibly becoming outdated in the paper). - 2. I found it strange to have the "The SMT-LIB specification language" featured so prominently in the paper, only to conclude by stating that knowledge of SMT-LIB is not needed to use the package. It might be worth demoting this section to another part of the paper, in order to focus on your own work.
The Repository
- 1. The first example is broken do to a wrong quotation character. This is fixed here.
- 2. The third example is broken, see the issue.
- 3. Please create a
CONTRIBUTING.mdfile with contribution guidelines, which will be useful for those contributing to your open issues. - 4. Similarly, please consider using github issue templates for bug reporting.
The Code
- 1. Running the tests produces some Error and Warning output, even though all the tests pass. Is this expected?
- 2. There are plenty of examples in the
examplesfolder, but these are not tested. You might want to add unit tests that run each example and check that no warnings are produced, to ensure that they don't become outdated inadvertently. - 3. You have great test coverage already. I see that most of the lines missing coverage are the if statements for error handling; Julia does provide unit testing for errors through the
@test_throwsmacro. It wouldn't take long to test that the error handling behaves as expected, and it would bring your coverage way up!
The Documentation
- 1. It seems that a recent update means no solvers have to be installed before using your package. However, the documentation still features installation instructions for solvers. Might this be clarified?
- 2. There are even more examples in the documentation, which is helpful, but as far as I can see these are not tested either. You might want to use Doctests.
- 3. There are documentation examples,
examplesfolder examples, andpaper_examplesexamples, all which appear different. Would it make sense to maintain the same examples in the repository and the documentation? Also, since the JOSS paper currently features no examples, I assumepaper_examplesare destined for another publication? If that's the case, it might make sense to place them in a different repository.
- 1. No code examples are given in the paper. Might it be nice to include some of the examples from the
README.mdfile?
I do not recommend adding code examples to the paper since these might not work with future versions. Maybe a link to the examples in the repo is sufficient?
- 1. No code examples are given in the paper. Might it be nice to include some of the examples from the
README.mdfile?I do not recommend adding code examples to the paper since these might not work with future versions. Maybe a link to the examples in the repo is sufficient?
That's fair enough!
Sorry for the delay, busy week.
Paper
- I found it strange to have the "The SMT-LIB specification language" featured so prominently in the paper, only to conclude by stating that knowledge of SMT-LIB is not needed to use the package. It might be worth demoting this section to another part of the paper, in order to focus on your own work
This came about because some readers might not be familiar with SMT-LIB, however less focus on it sounds like a reasonable change.
Repository
The CONTRIBUTING.md file and issue templates can be added - great idea! I'll take care of that in the next couple days.
Code
Yes, the warnings are expected. Hopefully there is some way to suppress them during testing.
Thanks for your suggestion on running the examples as part of the tests. I agree this would prevent them from becoming out of date - same with the README. Does anyone know how to run code in a README automatically?
Documentation
Yes! this is a recent change and now the documentation needs to get fixed. Thank you for catching it.
The paper_examples are from a previous submission. I agree the examples should be better organized and can get on that.
Does anyone know how to run code in a README automatically?
I had a similar issue recently when developing ConsensusBasedX.jl. For the documentation examples, my solution was to keep the examples as full Julia script in a separate folder, to write a unit test to load and run each file and check that no errors occur, and then to modify the documentation build to inject each of these examples into the raw markdown files before the build process, using the custom syntax {{subfolder/file}} (here's an example). There are other ways of achieving the same (e.g. Literate.jl or Quarto), but I thought they were overkill for my particular need.
This unfortunately does not work for the README.md file. I have just submitted a small pull request that tests the code in the README.md file. The exact output is not tested, only that the julia blocks can be parsed as Julia code and run without error. Given that these are only simple examples, and the methods should already be tested elsewhere, I think that's sufficient quality control.
EDIT: CI on the pull request has failed for the same reason that I could not run all the README.md examples; namely, that Yices is not installed. I see three alternative solutions:
- To install Yices in the Github Actions CI (I have not the faintest idea how easy this would be).
- To remove the Yices example from README.md and keep it only in the Documentation (though eventually one would run into the same issue, how does one test the code example without installing Yices).
- To not test the README.md examples.
No solution is ideal, so I leave the decision up to you entirely.
EDIT 2: Turns out installing Yices in Github Actions was rather easy. Is the pull request solution acceptable to you now that all the tests pass?
Hi Rafael,
Yes, thank you so much for resolving this.
I've been struggling to respond in a timely manner because of my internship...I just can't scrape up any time to resolve these on weekdays.
Hi @rafaelbailo and @diehlpk,
I've addressed the various documentation issues (reorganizing examples, cleaning them up etc) and code issues, including increasing unittests via @test_throws and fixing instances where the error or warning was printed while running the unittest.
I also made some edits to the paper to spend less time discussing the SMT-LIB language. A silly question: where/how should I commit those edits?
Thank you,
Emi
Hi @elsoroka, thanks for that! I think the code and documentation changes should be committed to main, unless you have a specific need not to do so. Ultimately, the changes will have to be merged into main and a new release made before the paper is accepted. As for the paper changes, I would commit them to the joss-paper branch so that we can automatically compile the edited version of the manuscript.
OK @rafaelbailo I have committed the paper changes to joss-paper. The code and documentation changes have already been merged into main.
Does there need to be a new tagged software release? The latest tag was May 28 which was the latest change to software functionality. The documentation was automatically rebuilt with the new changes yesterday.
There's no need for a new release until the editor instructs it. This will be right before publication. I'll go over your changes ASAP!
Everything looks good, closing this as completed, I'll update the review issue as well. Thanks for all your work, and congratulations on a great project! 😄