search

astamm / nloptr

nloptr provides an R interface to NLopt, a free/open-source library for nonlinear optimization providing a common interface to a number of different optimization routines which can handle nonlinear constraints and lower and upper bounds for the controls.
https://astamm.github.io/nloptr/
Other
106 stars 34 forks source link

Efficiency and safety review part 2/2 #133

Closed aadler closed 1 year ago

aadler commented 1 year ago

Hello, @astamm.

While large, this is the second and last PR relating to a code review for efficiency and safety. It applies all the checks and enhancements described in #131 to the remaining files. It also converted one sapply to vapply for both type-safety and speed. Perhaps most importantly, it expands the test suite so that all files but 2 are covered 100% and those 2 are at least 90%. The file is.nloptr has 3 checks which I believe cannot be tripped by a regular call to nloptr as they are protected by .checkfunargs. They can only be triggered by manually creating a malformed nloptr object, and I ran into some frustrations trying to do that. The second file, nloptr.c, has some error branches that I cannot seem to trip from within R (out-of-memory, forced stop, bad NLOPT ARGVALUES) so they too remain untested.

There should be no changes at all to the API and the only user-visible changes should be to the documentation and one error message in nl.grad.

Going through the code gave me some ideas about future enhancements, such as exposing ranseed to more than just isres or considering if we need all three of nl.grad, nl.jacobian, and finite.diff, or whether it may make sense to port nl.grad to C since it is used so often, but I'll definitely be taking a break for a while. I will make one addition to this PR, and that is to add the URL to NEWS once it registers.

Thank you for your patience; I appreciate it!

astamm commented 1 year ago

That's awesome. Thank you very much @aadler. Merging. Glad to discuss your ideas for improvement. I will be giving them some thoughts as well and try to invest some time into the package soon. I will consider a release asap as well.