JOSS Review Comments #51
Description
Hello, and congrats on your JOSS submission! Overall, the package is in a very good state, the paper is well written, 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. Generally, it is advisable to include code examples in the README.md and the documentation, but not in the JOSS paper, as the interface might change and the examples might become outdated. However, this seems unlikely in your case; I encourage you to consider the point, but I won't insist on it.
- 2. There is, however, a small issue with the code examples; see my comments in the "Repository" section.
The Repository
- 1. Please add a link to the documentation in the repository's About section; here is a guide on how to do so.
- 2. I tried following the code examples given in README.md (as well as in the paper and the documentation). Seems that the use of
Ias a shorthand for the identity matrix requires me to import LinearAlgebra.jl. You do mention later on that LinearAlgebra is a dependency of the package, but the code examples should work as standalone pieces of code. Please addusing LinearAlgebrato the examples. - 3. The text in the Dependencies section of README.md is confusing; you don't "need to install" anything, as the Julia package manager install dependencies for you when you install the package via
]add MultiPrecisionArrays. Please clarify this. - 4. The community guidelines need to be improved. You can add issue templates (such as these, which were created automatically following this tutorial so that bug reports and other issues are more concise and targeted.
- 5. Related also to the community guidelines, I was surprised to see that you specifically ask potential contributors not to make pull requests in README.md! What is the reason for this?
The Code
- 1. An alternative solution to the missing
using LinearAlgebraon the examples would be to re-export some of the functionality of LinearAlgebra for the user's convenience. You can use the Reexport.jl package for this. If you just want to re-export the identity, you can addusing Reexport; @reexport import LinearAlgebra.Ito the Examples module. If you think that the users of your package are likely to need the full functionality of LinearAlgebra, you could instead addusing Reexport; @reexport using LinearAlgebrato your main module, but this might be overkill. - 2. I noticed there's exactly one line missing test coverage, and it's an
endstatement. I have no idea why it's not covered, when the loop code has been executed, so I suspect a bug in the coverage code. Out of curiosity, I have asked about this on the Julia discourse. - 3. There are many commented lines in the code. Some of these are explanatory (e.g. "
# Scale first column" insrc/Factorizations/hlu!.jl), but some are just old commented code (e.g. "# residterm ? tf=1.0 : tf=.9" insrc/Solvers/mpkrir.jl). The latter are not acceptable, as they only clutter the code and confuse the user, and are unnecessary, as old code can always be recovered thanks to version control. If you find the commented old code helpful for development, I invite you to create a separate development branch where the code can be "messy", but contributions to the main branch should always be "tidy" for the sake of code readability. - 4. There is no consisting code formatting across the repository. You should be using JuliaFormatter, or an equivalent tool, to enforce consistent formatting on your main branch. One easy way to do so (not necessarily the optimal way) is to add a format command to your unit tests, such as this one, which enforces a given configuration across the entire codebase; I find this approach helpful as I always manually run my unit tests before committing changes to a branch.
The Documentation
- 1. There is, again, a small issue with the code examples; see my comments in the "Repository" section.