Closed whitehatty closed 5 years ago
Agreed. Anyone want to make a PR ? :1st_place_medal:
I can after work
On Oct 16, 2018, at 3:30 PM, Ben Letham notifications@github.com wrote:
Agreed. Anyone want to make a PR ? 🥇
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.
Pushed to PyPI.
@bletham Can we reopen this? I guess some other PR removed the fix, in v0.4 and master, lines 1096--1104, in forecaster.py:
try:
params = model.optimizing(**args)
except RuntimeError:
if 'algorithm' not in args:
# Fall back on Newton
args['algorithm'] = 'Newton'
params = model.optimizing(**args)
else:
raise
@whitehatty the behavior is a bit different, but it's now intentional and not a bug.
The bug was that previously if RuntimeError, we would repeat the optimization with the Newton optimizer like
model.optimizing(algorithm='Newton', **args)
which would throw a SyntaxError error if we had previously specified the algorithm in args
. This was a bug because we were passing in a repeated keyword argument.
Now, if algorithm
is specified, then it will be used in the first call to model.optimizing
. If that errors out with a RuntimeError, then we do not replace the algorithm, rather we let it raise. The thought here is that if the user goes to the effort of specifically setting algorithm='L-BFGS'
, then I don't think we should override that. If the user wants to have this fallback behavior they can not specify the algorithm, or they could handle the RuntimeError in some different way if they wanted.
Does that seem reasonable, or is there a use case that I'm missing?
My thought is that we should raise only if we can't fit the model, not if a specific algorithm fails, and this branch of code is invoked as a backup case anyway.
I am not sure why one would not want the model fitted by the Newton
method as a fitted model and consider raising an exception better. The only reason being
Newton
method is 10x slower (or some x slower)
Can you provide any insight on the last two possible issues?
In my specific case for example I am explicitly setting BFGS
because for some unknown reasons it converge faster than LBFGS (also appreciate if you have an explanation for that).
I see, so you're specifying BFGS, in which case it is reasonable to want to fall back to Newton. I was thinking that maybe you were specifying L-BFGS.
That makes sense, I opened a new issue in #877 to do this. (Since it is a slightly different issue than the one here, which was a bug).
As for BFGS requiring fewer iterations, that makes sense to me since L-BFGS uses an approximate Hessian instead of the full one that BFGS does in order to reduce memory usage. I haven't personally tested the fitting with BFGS to see what the memory/speed tradeoff is (L-BFGS is Stan default), but that seems worth doing.
In
forecaster.py
, lines 1046-1053:if kwarg algorithm is passed to fit and a RuntimeError is triggered the call to optimizing will fail due to duplicated kwarg algorithm. algorithm should be removed from kwargs before passing it to the call to optimizing.