Review from JOSS

[dourouc05]

Related to openjournals/joss-reviews#9467 (comment)

I've got a few suggestions for this package overall. This is currently WIP!

For the docs:

• Formatting in https://jso.dev/JSOSolvers.jl/stable/internal/: projected_line_search_ls! and cauchy_ls! do not have the same formatting as the other functions here.
• User friendliness in https://jso.dev/JSOSolvers.jl/stable/solvers/: if you stumble upon the package, it's unlikely you will understand NLP/NLS.
• https://jso.dev/JSOSolvers.jl/stable/solvers/#JSOSolvers.lbfgs: maybe a link on GenericExecutionStats/SolverCore.jl?
• As you have callbacks for every solver, maybe the corresponding explanations could be made into their own section?
• Is there a link from the docs to the tutorials (https://jso.dev/tutorials/)?

Overall, the docs are quite good!

In the paper:

• "Julia’s native C++ and Fortran interoperability": I didn't know about C++, but the C part is quite good.
• Benchmarking: I would find it interesting if you could benchmark against the canonical implementations that you are referring to in the paper (like the original TRON code). I understand it might be quite a bit of work, hence feel free to ignore! Benchmark TRON with MINRES as subsolver vs CG #306

Code:

• I don't really like the way you handle the Krylov subsolvers. I know that you take the same API as the underlying library, but it doesn't make it easy to swap this part of the algorithm (although it would seem, for a user perspective, doable, and potentially useful, such as preconditioning Integrating a preconditioner into JSOSolvers subsolvers #281). It'd be great to think about potential solutions for v1.0 (but not mandatory at all for this publication!). Benchmark TRON with MINRES as subsolver vs CG #306

A really nice package, a great fit for JOSS!

[tmigot]

Thanks @dourouc05 for the comments. I’ve opened a separate PR to address the paper and the documentation.

Regarding the code question: if you think it’s appropriate, I can open a separate issue to discuss this further. There are already related issues, such as #281 and #306.

Finally, for benchmarking against other implementations, I would also suggest opening a new issue. We are currently working on running benchmarks via GitHub Actions with clean comparisons and reproducible results. Once this is in place, it will be much easier to publish benchmarks with other solvers directly in the documentation.

[dourouc05]

I ticked the boxes for the changes for which you have opened PRs that I have approved. (I hope that this doesn't make me an approver for this package :D!)

For the other changes, feel free to open other issues (for instance, referring to this one) as you see fit. For the code, I think you have issues that cover what I mean, which is enough.

[tmigot]

I opened a new targeted issue for the benchmark, #342.
A pull request for the callback #341
Regarding the link from the doc to the tutorials, the Examples section in the doc is doing it: https://jso.dev/JSOSolvers.jl/latest/examples/ . Do you think it's enough?