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

ENH: add differential-evolution to minimize #10778

Closed
wants to merge 6 commits into from
Closed

Conversation

andyfaff
Copy link
Contributor

@andyfaff andyfaff commented Sep 6, 2019

This PR adds a 'differential-evolution' method to minimize.

@andyfaff
Copy link
Contributor Author

andyfaff commented Sep 6, 2019

@mdhaber, you seem to be an RST expert. Can you tell from inspecting the changes I made to _minimize where the doc build is going wrong?

@@ -77,6 +78,7 @@ def minimize(fun, x0, args=(), method=None, jac=None, hess=None,
- 'trust-ncg' :ref:`(see here) <optimize.minimize-trustncg>`
- 'trust-exact' :ref:`(see here) <optimize.minimize-trustexact>`
- 'trust-krylov' :ref:`(see here) <optimize.minimize-trustkrylov>`
- 'differential-evolution' :ref:`(see here) <`scipy.optimize.differential_evolution`>`
Copy link
Contributor

@mdhaber mdhaber Sep 6, 2019

Choose a reason for hiding this comment

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

Maybe here?

Suggested change
- 'differential-evolution' :ref:`(see here) <`scipy.optimize.differential_evolution`>`
- 'differential-evolution' :ref:`(see here) <optimize.differential_evolution>`

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that might not be all:

/home/circleci/repo/build/testenv/lib/python3.7/site-packages/scipy/optimize/_minimize.py:docstring of scipy.optimize.minimize:41: WARNING: undefined label: (see here) <`scipy.optimize.differential_evolution (if the link has no caption the label must precede a section header)
/home/circleci/repo/build/testenv/lib/python3.7/site-packages/scipy/optimize/_minimize.py:docstring of scipy.optimize.minimize:309: WARNING: undefined label: scipy.optimize.differential_evolution (if the link has no caption the label must precede a section header)
/home/circleci/repo/build/testenv/lib/python3.7/site-packages/scipy/optimize/optimize.py:docstring of scipy.optimize.show_options:51: WARNING: undefined label: scipy.optimize.differential_evolution (if the link has no caption the label must precede a section header)

I'll look into it but hah I'm no expert. I just had the misfortune of fighting with it a lot when working on the contributor guide.

Copy link
Contributor

@mdhaber mdhaber Sep 6, 2019

Choose a reason for hiding this comment

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

Following https://github.com/scipy/scipy/pull/8328/files as an example of all the changes made to add a method to minimize, it looks like you might need to add a file analogous to:
scipy/doc/source/optimize.minimize-trustconstr.rst

That file seems to define the reference like:
.. _optimize.minimize-trustconstr:
that is referred to like:
:ref:(see here) <optimize.minimize-trustconstr>`

I should update the contributor guide with this stuff.

@@ -326,6 +330,9 @@ def minimize(fun, x0, args=(), method=None, jac=None, hess=None,
used to solve the subproblems with increasing levels of accuracy
as the iterate gets closer to a solution.

Method :ref:`differential-evolution <scipy.optimize.differential_evolution>` uses
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Method :ref:`differential-evolution <scipy.optimize.differential_evolution>` uses
Method :ref:`differential-evolution <optimize.differential_evolution>` uses

@@ -3008,6 +3008,7 @@ def show_options(solver=None, method=None, disp=True):
- :ref:`SLSQP <optimize.minimize-slsqp>`
- :ref:`dogleg <optimize.minimize-dogleg>`
- :ref:`trust-ncg <optimize.minimize-trustncg>`
- :ref:`differential-evolution <scipy.optimize.differential_evolution>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- :ref:`differential-evolution <scipy.optimize.differential_evolution>`
- :ref:`differential-evolution <optimize.differential_evolution>`

@mdhaber
Copy link
Contributor

mdhaber commented Sep 6, 2019

It looks like there is usually an addition to scipy/optimize/__init__.py when adding methods, too.

@andyfaff
Copy link
Contributor Author

andyfaff commented Sep 6, 2019

Let's see if this works. I added the extra rst file. However, in this form the documentation for differential_evolution will show up in full when one clicks on the link for the method, so you get its version of the func/bounds docstring.

@andyfaff
Copy link
Contributor Author

andyfaff commented Sep 7, 2019

All green. I'll can add dual_annealing, etc, in a subsequent PR.

@rgommers
Copy link
Member

rgommers commented Sep 7, 2019

@andyfaff was this discussed somewhere? I haven't followed this, but intuitively it seems a little odd to add a global optimization as one of the methods in the interface for local optimization.

@andyfaff
Copy link
Contributor Author

andyfaff commented Sep 7, 2019

@rgommers, no it wasn't discussed, putting it on the mailing list is probably a good idea.

I guess I don't really see them as local vs global minimizers as such. To me they're all do the same thing, multivariate scalar minimisation, just with different flavours for finding the minimum.

basinhopping/differential_evolution/shgo/dual_annealing all use the same kind of input as the 'local' minimizers. I thought it would be nice if they could use the same minimize interface as well, just by swapping the method kwd.

Perhaps the local <--> global distinction in the docs doesn't need to be as pronounced as it is. There could be a reorg of the page to have 'local' minimize methods and 'global' minimize methods.

I've got no particular axe to grind here, apart from thinking it would be nice to use them through the minimize function.

@rgommers
Copy link
Member

rgommers commented Sep 7, 2019

I guess I don't really see them as local vs global minimizers as such. To me they're all do the same thing, multivariate scalar minimisation, just with different flavours for finding the minimum.

Hmm, at a very high level perhaps. There's a reason that the global optimizers call local ones to improve the results once an approximate minimum has been found. I think it's quite an important distinction for users to understand. Let's see what others think.

@mdhaber
Copy link
Contributor

mdhaber commented Sep 8, 2019

I think it's an important distinction for users to understand, but I don't think we need to artificially enforce it by avoiding a unified interface. The problems are specified in the same way, and the global solvers could use a unified interface, so I wouldn't mind bringing them into minimize.

@rgommers
Copy link
Member

@andyfaff please feel free to push this forward, if others think it's a good idea or at least are not -1 then it's fine with me. I would suggest to mention it on the mailing list as I said above.

@andyfaff
Copy link
Contributor Author

Force pushed to resolve merge conflicts

@tupui
Copy link
Member

tupui commented Feb 6, 2021

What about having 3 functions:

  • local_minimize
  • global_minimize
  • minimize call one or the other.

This would allow to keep the logical separation between the two classes (which is paramount IMO) and still provide the end user with a simple interface. You could even have a flag to call local or global, etc.

@andyfaff andyfaff closed this Sep 20, 2022
@tupui
Copy link
Member

tupui commented Sep 20, 2022

@andyfaff You closed because you don't want that we do this anymore or you just don't want to do this yourself?

@andyfaff
Copy link
Contributor Author

I think it would work well in minimize, but it didnt seem there was much groundswell for it.

@tupui
Copy link
Member

tupui commented Sep 20, 2022

As pointed out it would be good to discuss this further with others, so going through the mailing list.

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