Closed odunbar closed 6 months ago
First of all, please accept my apologies for taking this much time to respond your review comments.
To answer your General Checks remarks, the other authors did not actually contribute to the code per se but they were important stakeholders in the conception of this package which is why we thought relevant to refer them as authors. To bring some details, Alain Marcotte helped me to carry out the comparison tests between Julia and Fortran and the other two supervised the conduct of this project. As you already kind of guessed, the code was first developed on a local repository and then transfered to my lab GitHub when we published it on the Julia package manager, which explains the small number of commits and the current versioning. I hope this clarifies the background of the package!
Thanks a lot for your suggestions of improvement. I started to bring corrections and additions to the package source code and documentation in accordance with your remarks and will come back to you once I finished completing them as you suggested.
I am, once again, sorry for my response delay.
No problem! Thanks for onboarding the feedback
I modified the documentation by adding a description of the method and correcting typos. I also changed the status
docstring eventhough it is not visible in the documentation page yet.
I am currently working with Hydro-Québec to provide an industrial use of our package.
Hi! I pushed a revised version of the paper. A brief description of the method and its theoretical guarantees have been added. There is also a paragraph about an Hydro-Québec application.
Thanks @pierre-borie
I think I have ticked off everything that I can clearly see have been completed. I also understand that the HQ example is tough to use due to the proprietary parts.
@odunbar - Thanks for your positive feedback on the new version. From what I can see, there is just the "diagnostics" issue that has not been checked though the corresponding functionality has been added but it might be my fault. I wanted to mention this issue specifically but did wrong somewhere and tried to backtrack.
I corrected the paper accordingly to your last items and added a table for Chained Rosenbrock problem showing time for different dimensions on the dev version of the documentation.
For now, I am waiting for the other reviewer's comments before submiting a revised version.
OK great! I like the table, thank you for the response and I'll take your word for it on the software paper typo's
Purpose
Issues found during covering the checklist: https://github.com/openjournals/joss-reviews/issues/6226 Please address these either by return message in this thread, or by adding pull-requests/commits tagging these issues. I'll check of these points when satisfied!
Overall Impression
I see this as a usable, light, and compact tool in Julia. Certainly it is worthy of being a registered package. However, for a JOSS paper, more evidence of package usefulness in application is required. I believe we could get a JOSS publication here, but there needs to be some more proof, in the form of examples/performance tests of what is implemented, and the software paper should state claims reflecting what is actually present in the codebase.
General Checks
Functionality
r_i
orc_i
(3) Showcase a test problem from the Hydro-Quebec application. (4) A guides to the user for when the code is more/less performant or plots of how it scales with increasing numbers of equations/constraintsFloat64
. could this be extended to work with any precision (<:AbstractFloat
). Likewise, the provided functions appear to need to take in and return specific types which is not clear. For examples, the Jacobians appear to need to take inVector{Float64}
and returnMatrix{Float64}
(if i interpret the example correctly), could such restrictions be made more clear for users.Documentation
index.md
x \in R^n
not givenstatus
could be expanded. what are all the options and what do they mean?Software paper
lpopt.jl
? This I think solves all the constrained problems too?model
as it is not simply returned bysolve!(model)
Misc
main
notmaster
, (it is a simple change for developers)From review corrections
[
]
in autodifferentiation referencesolution(model)
gives the user the solutionn
that users may encounter. I think perhaps a table with just time to solution forn=10
,n=100
,n=1000
, (...n=10^4
if that's possible...) over 10 repeats would be great.