Skip to content

Conversation

kyleabeauchamp
Copy link
Contributor

  1. This adds the failing test described in MLE gives incorrect value with Uniform RVs #2482
  2. This builds on fixes to find_MAP #2468, primarily because it has some syntactic simplifications for grabbing transformed vs untransformed output of find_MAP().

bwengals and others added 3 commits August 1, 2017 18:39
- float32 support
- different arg, method="L-BFGS-B" instead of fmin=optimize.fmin_l_bfgs_b
- allow keyboard interrupt, save current value
- returns untransformed and transformed variables
- removes monitor, adds tqdm progress bar
vars : list
List of variables to set to MAP point (Defaults to all continuous).
fmin : function
Optimization algorithm (Defaults to `scipy.optimize.fmin_bfgs` unless
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to deprecate fmin rather than getting rid of it completely right away? This might cause a fair amount of breakage. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think deprecate it is a better idea. @bwengals

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just look for it in kwargs and warn if its there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to re-work this PR either choice on the breakage, but there wasn't really a decision made in #2468...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's easier after #2468 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for fmin in kwargs would work I think. Should it warn and then run?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually maybe its a bit trickier than I thought. different fmin functions have slightly different APIs. According to its docstring, fmin_bfgs doesnt support a cost function that returns both the value and gradient, while fmin_l_bfgs_b does.

@kyleabeauchamp
Copy link
Contributor Author

I've closed this PR and opened it as a PR into @bwengals branch for #2468

@kyleabeauchamp
Copy link
Contributor Author

So we don't have to comment on the same API changes twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants