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

Differential evolution #1062

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Differential evolution #1062

merged 3 commits into from
Jan 2, 2024

Conversation

NAThompson
Copy link
Collaborator

No description provided.

@NAThompson NAThompson marked this pull request as draft December 22, 2023 20:16
@NAThompson NAThompson force-pushed the differential_evolution branch 23 times, most recently from 71c3dfc to a4652f9 Compare December 25, 2023 23:48
@NAThompson NAThompson marked this pull request as ready for review December 26, 2023 00:24
@NAThompson
Copy link
Collaborator Author

@jzmaddock , @mborland : The only thing I'd like to change about this is the fact it can only take one argument type. I've resigned myself to the fact that each argument must be a container, but I would like to be able to query with (say)

length_args = std::vector<boost::units::meters>{....};
length_args = std::vector<boost::units::seconds>{....};
auto cost = cost_function(length_args, time_args);

This seems . . . somewhat doable.

include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
include/boost/math/tools/differential_evolution.hpp Outdated Show resolved Hide resolved
@jzmaddock
Copy link
Collaborator

I'm not sure I know enough about the problem domain to comment in detail (except that we do need this!), but I worry somewhat about the interface to argmin, specifically the need to explicitly specify the ArgumentContainer type via the ugly obj.template argmin<ArgumentContainer>() syntax (one bit of C++ I've never liked!).

I wonder, is it reasonable to suppose that ArgumentContainer is the same type as the bounds? That's not currently possible with the current interface, but if the type were templated on ArgumentContainer then the bounds could be two ArgumentContainer values: one for the minimum of each parameter, and one for the maximum.

Second question: does this need to be an object? Which is to say, does it get reused and/or cache values? Or should it be a free function? I appreciate there are a lot of parameters though!

On the docs front, it would be useful to have at least one usage example in the docs, rather than just pointing to the test cases.

@NAThompson
Copy link
Collaborator Author

NAThompson commented Dec 27, 2023

@jzmaddock :

but I worry somewhat about the interface to argmin, specifically the need to explicitly specify the ArgumentContainer type via the ugly obj.template argmin() syntax (one bit of C++ I've never liked!).

Yeah it's horrible. I think I'll do the Julia thing and just store all the params in a struct (this was also a suggestion in the codereview), and then I'll just make it a free function.

I wonder, is it reasonable to suppose that ArgumentContainer is the same type as the bounds? That's not currently possible with the current interface . .

Is it impossible? I wonder if we could say BoundsContainer = std::vector<std::array<Real,2>> then implies ArgumentContainer is std::vector<Real>, or if BoundsContainer= std::array<std::array<Real,2>, n> then ArgumentContainer must be std::array<Real, n>. Admittedly my template-fu is not sufficient to achieve this but it does seem doable.

Nonetheless, your suggestion to just provide two arrays does indeed solve the ugliness problem and is a clear win over what currently exists.

Final question: Would it make sense to create a boost/math/optimization directory? boost/math/tools seems to be quite overloaded . . .

@mborland
Copy link
Member

What about both a free function that takes a structure that unpacks into one that just takes the params list like the scipy signature?

@NAThompson
Copy link
Collaborator Author

@mborland : Yeah that's probably the right approach . . .

@jzmaddock
Copy link
Collaborator

Some good suggestions in the review there... it's possible to take the parameter structure a step further, and create a function with named arguments: https://pdimov.github.io/blog/2020/09/07/named-parameters-in-c20/ The drawback is that the parameters have to appear in the correct order, but defaulted ones can be omitted.

Is it impossible? I wonder if we could say BoundsContainer = std::vector<std::array<Real,2>> then implies ArgumentContainer is std::vector, or if BoundsContainer= std::array<std::array<Real,2>, n> then ArgumentContainer must be std::array<Real, n>. Admittedly my template-fu is not sufficient to achieve this but it does seem doable.

Yes definitely doable I think, requires a traits class with a template-type template parameter to extract the contained type:

template <class Container> struct rebind_container;

template <template <typename...> class Container, class T1, class T2>
struct rebind_container<Container<T1, T2>>
{
   using type = Container<typename T1::value_type, T2>;
};

@NAThompson NAThompson force-pushed the differential_evolution branch 4 times, most recently from c8527a8 to 171abb3 Compare January 1, 2024 06:38
@NAThompson
Copy link
Collaborator Author

NAThompson commented Jan 1, 2024

@mborland , @jzmaddock : Example added, class changed to free function + parameter struct, C++20 named parameters can be used by user (though it works with C++17), docs written.

The only thing I'd really like to see is real-time observation of the progress in an example, but I think that's out of reach without a GUI dependency.

BTW anyone know why only Windows+gcc-8 hates the Func template syntax?

ifferential_evolution_test.cpp: In instantiation of 'void test_rosenbrock_saddle() [with Real = double]':
differential_evolution_test.cpp:138:34:   required from here
differential_evolution_test.cpp:72:45: error: no matching function for call to 'differential_evolution(<unresolved overloaded function type>, boost::math::tools::differential_evolution_parameters<std::array<double, 2> >&, std::mt19937_64&)'
   auto local_minima = differential_evolution(rosenbrock_saddle<Real>, de_params, gen);
                       ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from differential_evolution_test.cpp:10:
..\..\../boost/math/tools/differential_evolution.hpp:155:19: note: candidate: 'template<class ArgumentContainer, class Func, class URBG> ArgumentContainer boost::math::tools::differential_evolution(Func, const boost::math::tools::differential_evolution_parameters<ArgumentContainer>&, URBG&, std::invoke_result_t<Func, ArgumentContainer>, std::atomic<bool>*, std::vector<std::pair<ArgumentContainer, typename std::invoke_result<Func, ArgumentContainer>::type> >*, std::atomic<typename std::invoke_result<Func, ArgumentContainer>::type>*)'
 ArgumentContainer differential_evolution(
                   ^~~~~~~~~~~~~~~~~~~~~~
..\..\../boost/math/tools/differential_evolution.hpp:155:19: note:   template argument deduction/substitution failed:
differential_evolution_test.cpp:72:45: note:   couldn't deduce template parameter 'Func'
   auto local_minima = differential_evolution(rosenbrock_saddle<Real>, de_params, gen);
                       ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@NAThompson NAThompson force-pushed the differential_evolution branch 6 times, most recently from 59f9c02 to c3ca8e6 Compare January 1, 2024 22:49
@NAThompson
Copy link
Collaborator Author

@mborland, @jzmaddock : Ain't gonna lie, I'm sick of this now! Feel free to add comments if you see anything else that needs addressing and I'll get to it in another PR.

@NAThompson NAThompson merged commit 4ee8391 into develop Jan 2, 2024
60 of 61 checks passed
@NAThompson NAThompson deleted the differential_evolution branch January 2, 2024 01:09
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.

None yet

3 participants