Proposed fix for bug 17703. #202
Conversation
src/library/stats/R/constrOptim.R
Outdated
| obj <- f(theta, ...) | ||
| if (s.mu * obj > s.mu * obj.old) break | ||
| } | ||
| if (optim_failure) { | ||
| a$convergence <- 21 # See https://github.com/nashjc/optimx/blob/main/man/optimx.Rd | ||
| a$message <- gettextf("Returning solution from outer iteration %d, either because the solution is not in the feasible region or `optim()` provided non-finite outputs. Consider either checking the gradient implementation or using a derivative-free opimizer or reducing `outer.eps`.", i) |
src/library/stats/R/constrOptim.R
Outdated
|
|
||
| a <- optim(theta.old, fun, gradient, control = control, | ||
| method = method, hessian = hessian, ...) | ||
| r <- a$value | ||
| if (is.finite(r) && is.finite(r.old) && | ||
| abs(r - r.old) < (1e-3 + abs(r)) * outer.eps) break | ||
| theta <- a$par | ||
| totCounts <- totCounts + a$counts | ||
|
|
||
| if (any(ui%*%theta-ci<0) || !is.finite(r) || any(!is.finite(theta))) { |
There was a problem hiding this comment.
This is strange, the indentation looks fine in local...
There was a problem hiding this comment.
this could be due to the R sources' inconsistent use of tabs and spaces. Be sure to enable visible whitespace in your editor to help clarify. Generally, I recommend using non-invasive IDEs to work on the R sources to minimize diffs (I typically just use nano, for example, though emacs is also widely used and does better at respecting nearby whitespace choices).
There was a problem hiding this comment.
That was indeed the issue, thank you for your help! I thought neovim would also respect nearby whitespace choices, but apparently it does not :/
|
I think the use of code "21" is heroic. In the .Rd file for optimr I have "Various methods may or may not return sufficient information to allow all the codes to be specified." In fact, while I use the codes if produced by the solvers, the majority don't return most of them. That's probably a great topic for a GSoC or similar project, but getting the codes into the solvers would be a pretty massive job, and need injection of code in all sorts of programming languages and interaction with other projects. In particular, I think only my own Rvmmin or its update to nvm will give the 21 code. They could be used when there is a gradient function available. In fact, they'll complain and stop if one is not provided. This would mean changing "BFGS" to "nvm" in the "method = " line for a gradient method. This should be an easy change, but I'm sure there'll be some glitches. Unfortunately .... the results won't quite match BFGS so you may get revdep hell in commit to CRAN. |
|
I added tests, I tried to find the right initial parameters to reproduce the bug (that would now return a convergence code 21). |
|
I posted a comment. The 21 code likely only comes from my Rvmmin and nvm.
JN
…On 2025-05-23 13:29, Léopold Guyot wrote:
*leopoldguyot* left a comment (r-devel/r-svn#202) <#202 (comment)>
I added tests, I tried to find the right initial parameters to reproduce the bug (that would now return a convergence
code 21).
But it seems like that the reproducibility is also OS/machine dependent. Should I remove the tests?
—
Reply to this email directly, view it on GitHub <#202 (comment)>, or
unsubscribe <https://github.com/notifications/unsubscribe-auth/
AA247LDWQDA7IKU2VAAUSIL275LHBAVCNFSM6AAAAAB5WGVAUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMBVGIZTKNJZGY>.
You are receiving this because you commented.Message ID: ***@***.***>
|
What would you suggest? Should we return a convergence code of 0 (already returned by Thank you for your help :) |
|
The difficulty is that the different solvers return an array of termination codes.
Rvmmin and successors are able to detect extreme gradient and return the 21 code. optim() codes
are more than half a century old and don't.
There is an (unfortunately not fully finished) spreadsheet in optimx package inst/doc/opt/optcontrol.xls
listing convergence codes that I've tried to return through the function optimr().
Of all the issues with optimx, controls and convergence codes have been the most annoying.
As I indicated in earlier post, you could try using method="nvm" if optimx is used with constrOptim.
However, as constrOptim part of the stats package, that is likely going to be difficult.
One of the difficulties with R is that stuff in the stats package rarely gets killed off, while more
modern functions are booted out of CRAN when there are minor glitches.
One possibility is to actually ditch the call to optim for the gradient case and put in the code
from nvm directly. If I were doing this, I'd first require optimx to get nvm and test using method="nvm".
If it works, the code from optimx could be incorporated in constrOptim.
JN
…On 2025-05-26 05:11, Léopold Guyot wrote:
*leopoldguyot* left a comment (r-devel/r-svn#202) <#202 (comment)>
I posted a comment. The 21 code likely only comes from my Rvmmin and nvm. JN
… <#>
On 2025-05-23 13:29, Léopold Guyot wrote: /leopoldguyot/ left a comment (r-devel/r-svn#202 <https://github.com/r-
devel/r-svn/pull/202>) <#202 (comment) <#202 (comment)>> I added
tests, I tried to find the right initial parameters to reproduce the bug (that would now return a convergence code
21). But it seems like that the reproducibility is also OS/machine dependent. Should I remove the tests? — Reply to
this email directly, view it on GitHub <#202 (comment) <https://github.com/r-devel/r-svn/
pull/202#issuecomment-2905235596>>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ <https://
github.com/notifications/unsubscribe-auth/>
AA247LDWQDA7IKU2VAAUSIL275LHBAVCNFSM6AAAAAB5WGVAUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMBVGIZTKNJZGY>. You are
receiving this because you commented.Message ID: /*@*/.***>
What would you suggest? Should we return a convergence code of 0 (already returned by |optim|) or should we create a new
code and define it in the documentation?
Thank you for your help :)
—
Reply to this email directly, view it on GitHub <#202 (comment)>, or
unsubscribe <https://github.com/notifications/unsubscribe-auth/
AA247LHX4KLWHTYVLOSG2633ALLFDAVCNFSM6AAAAAB5WGVAUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMBZGAZDOMJZGM>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
Sorry: should point to optimx package inst/doc/optcontrol.xls Extra characters in post above. |
|
FWIW, the attached try doesn't croak, but I've not checked if results are sane. |
|
If it turns out that the use of nvm rather than BFGS is "interesting", I'm willing to work with others to get nvm code directly into constrOptim, since that function is in stats library and not a package. It is about 4* as long a code as constrOptim, so tail wagging the dog. |
|
Hi John thanks for the idea about nvm, but let's leave that for a separate issue/discussion. |
|
It may also be feasible to create a function, say "cograd()" that wraps the gradient function of the user and tests for extreme values. That would not be a huge amount of code and could be a sensible workaround. The tricky part might be to pass back the failure information to BFGS and escape. |
Sure but I will not be able to do it before friday 😅 |
|
@leopoldguyot can you make the report back to Bug 17703 to summarise your work here and what you think the next steps should/could be? |
|
Hello, sorry everyone for the long wait, it completely slipped my mind. I've submitted the patch. |
|
looks great thanks! |
|
Nice ! Thanks @leopoldguyot ! |
|
Fixes like this are important to keep the R environment working. |
A proposed solution to bug 17703 from group effort with @tdhock @leopoldguyot @ElsLommelen.