-
-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAINT: avoid redundant evaluation of user-provided function in l-bfgs-b optimization #4083
Conversation
|
||
""" | ||
if args is None: | ||
args = () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you make ()
the default value for args
? Or does it actually get None
passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it like that because I knew there was some gotcha for []
as default value, but I guess this would not apply to args that are not mutable, so I'll change the default to ()
.
I'd imagine that at least some of the other minimization routines are also effected by this. Its probably worth making this change to them as well. |
I agree that the other minimization functions should be similarly changed if they suffer from similar issues, but I'm not sure I'll be up for digging through that code any time soon. Should this be required as a condition for merging the PR? |
ei[k] = 0.0 | ||
return grad | ||
|
||
|
||
def approx_fprime(xk, f, epsilon, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered adding **kwargs
to approx_fprime
instead of adding this helper function?
I seems to me that somthing like that would work:
def approx_fprime(xk, f, epsilon, *args, **kwargs):
f0 = kwargs.pop('f0', None)
if kwargs:
raise TypeError('unepected keyword arguments: %s' % ', '.join(kwargs.keys()))
if f0 is None:
f0 = f(*((xk,) + args))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not... if *args
means 'user-supplied args to the user-supplied callback functions' then shouldn't **kwargs
mean 'user-supplied kwargs to the user-supplied callback functions'? Of course as I mentioned I'd rather just get rid of user-supplied args altogether and make them use functools.partial
or some other callable instead, but this is not realistic in terms of backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easiest to not go changing approx_fprime
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, I'm -1 to adding **kwargs
for the purpose of including f0
, and I'm also -1 to adding **kwargs
for the purpose of allowing user-supplied **kwargs
to the user functions. I'm also -1 to removing the *args
but only because of backwards compatibility, otherwise I'd be +1. So I agree with not changing the interface of approx_fprime
.
MAINT: avoid redundant evaluation of user-provided function in l-bfgs-b optimization
closes #4076