Skip to content
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

fix passing of kwargs to optimizers #5815

Merged
merged 2 commits into from Feb 16, 2017
Merged

fix passing of kwargs to optimizers #5815

merged 2 commits into from Feb 16, 2017

Conversation

nden
Copy link
Contributor

@nden nden commented Feb 15, 2017

Closes #4611

@bsipocz bsipocz added this to the v1.3.1 milestone Feb 15, 2017
@bsipocz
Copy link
Member

bsipocz commented Feb 15, 2017

@nden - Could you please add a changelog entry?

@nden
Copy link
Contributor Author

nden commented Feb 16, 2017

@bsipocz done and merging

@nden nden merged commit eefef16 into astropy:master Feb 16, 2017
bsipocz pushed a commit that referenced this pull request Feb 22, 2017
fix passing of kwargs to optimizers
@ycopin
Copy link
Contributor

ycopin commented Apr 7, 2017

I think these changes somehow break previous behavior, and should therefore be stated more explicitly. I used to propagate the verbosity flag disp in the kwargs (the one expected by scipy.optimize.fmin_slsqp), and this now ends up in error with:

  File "/data_sys/ycopin/Dropbox/Science/Euclid/Emilines/statistics.py", line 129, in __call__
    self.objective_function, p0, farg, **kwargs)
  File "/data/ycopin/Softs/local/lib/python2.7/site-packages/astropy/modeling/optimizers.py", line 231, in __call__
    full_output=True, **kwargs)
TypeError: fmin() got multiple values for keyword argument 'disp'

One should now use the verblevel keyword, as mentioned in the doc. Wouldn't it be more sensible to change the doc to match last version's working behavior?

@nden
Copy link
Contributor Author

nden commented Apr 7, 2017

@ycopin Sorry, my bad. The goal was to use the same keyword with all fitters but I see that in fixing one problem I created a different one.
Given the state of things would it be sufficient to allow either verblevel or disp as input to SLSQP?

@ycopin
Copy link
Contributor

ycopin commented Apr 8, 2017

I think it would be better to simplify the code, by allowing a single well-documented keyword, and stick to it, and letting people know, maybe with a DeprecationWarning. My 2 cents...

@pllim
Copy link
Member

pllim commented Apr 10, 2017

I think this should be a new issue, otherwise it is buried in a merged PR and hard to find.

@nden
Copy link
Contributor Author

nden commented Apr 10, 2017

@pllim Agreed. I'll make a PR asap.

@nden nden deleted the modeling#4611 branch December 27, 2018 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants