-
Notifications
You must be signed in to change notification settings - Fork 160
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
Adding explicit MINLP support #115
Conversation
doc/sphinx/docs/algorithm_list.rst
Outdated
@@ -20,12 +21,12 @@ Differential Evolution (DE) :cpp:class:`pagmo::de | |||
Self-adaptive DE (jDE and iDE) :cpp:class:`pagmo::sade` :class:`pygmo.sade` S-U | |||
Self-adaptive DE (de_1220 aka pDE) :cpp:class:`pagmo::de1220` :class:`pygmo.de1220` S-U | |||
Particle Swarm Optimization (PSO) :cpp:class:`pagmo::pso` :class:`pygmo.pso` S-U | |||
(N+1)-ES Simple Evolutionary Algorithm :cpp:class:`pagmo::sea` :class:`pygmo.sea` S-U (sto) | |||
Simple Genetic Algorithm :cpp:class:`pagmo::sga` :class:`pygmo.sga` S-U (sto) | |||
(N+1)-ES Simple Evolutionary Algorithm :cpp:class:`pagmo::sea` :class:`pygmo.sea` S-U (sto) |
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.
Seems like an extra space slipped in here.
We found a feasible solution! | ||
|
||
.. note:: | ||
The solution strategy above is, in general, flawed in assuming the best solution of the relaxed problem is colse to the |
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.
"colse" -> "close"
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.
done
|
||
.. note:: | ||
The solution strategy above is, in general, flawed in assuming the best solution of the relaxed problem is colse to the | ||
the full MINLP problem solution. More sophisticated techniques would instead search the combinatorial part more exhaustvely. |
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.
"exhaustively"
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.
done
.. note:: | ||
The solution strategy above is, in general, flawed in assuming the best solution of the relaxed problem is colse to the | ||
the full MINLP problem solution. More sophisticated techniques would instead search the combinatorial part more exhaustvely. | ||
We used here this approach only to show how simple is, in pygmo, to define and solve the relaxed problem and |
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.
"How simple it is"
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.
done
@@ -109,7 +109,7 @@ After inspection, let us now run the evolution. | |||
array([ 4.07802374, -23.82020921, -4.07802374]), | |||
array([ 1.71396489, -25.90794514, -1.71396489]), | |||
...] | |||
>>> archi.evolve() | |||
>>> archi.evolve() #doctest: +SKIP |
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.
Why do this (and in a bunch of places below) need to be skipped as no output is being produced?
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.
It does not need to be skipped, I choose to skip it as it makes the test much longer and this call syntax is not likely to change ever.
include/pagmo/utils/generic.hpp
Outdated
retval[i] = uniform_real_from_range(bounds.first[i], bounds.second[i], r_engine); | ||
} | ||
if (nix > 0u) { |
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 don't think this is the right way to go.
First, we are hard-coding the integer type to int
via std::uniform_int_distribution<>
. This effectively means restricting the range of the integer variables to 32 bits on modern platforms. This seems a rather important (and undocumented) implementation characteristic which may be limiting (?).
Secondly, the static casts are going to result in undefined behaviour if the floating point values are outside the int
range.
What about instead generating numbers with a uniform real distribution in the specified range and calling something like std::round
on them? I'd like to avoid going through a conversion to a C++ integer type, as that it's a shitcan to deal with and in the end we are always dealing with floating point values in the bounds, dvs etc. anyway.
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.
Ok I then have to use floor and add 1 to the upper bound as to ensure the obtained distribution from int is uniform in the bounds.
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.
done
* | ||
* @throws std::invalid_argument if: | ||
* - the bounds are not of equal length, they contain NaNs or infs, or \f$ \mathbf{ub} < \mathbf {lb}\f$, | ||
* - the bounds are not of equal length, they have zero size, they contain NaNs or infs, |
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.
Missing failure modes.
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.
added (except infinites)
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.
done
pygmo/docstrings.cpp
Outdated
|
||
Raises: | ||
ValueError: if *nix* is negative or greater than an implementation-defined value | ||
ValueError: if *lb* and *ub* are malformed (unequal lenght, zero size, *nix* larger than len(lb) or bounds are not integers in their last *nix* components) |
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.
lenght
-> length
, missing failure mode, "len(lb)" should probably be in backticks as it's a fragment of code.
self.assertTrue(int(x[1]) != x[1]) | ||
set_global_rng_seed(42) | ||
y = random_decision_vector(lb = [1.1,2.1,-3], ub = [2.1, 3.4,5], nix = 1) | ||
self.assertTrue((x == y).all()) |
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.
Would be good to have a couple of tests for failure modes as well.
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.
those are tested in the C++ part. I usually only test the boost interface in test.py
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 would consider exception translation part of the boost interface.
tests/generic.cpp
Outdated
@@ -71,16 +71,33 @@ BOOST_AUTO_TEST_CASE(random_decision_vector_test) | |||
BOOST_CHECK_THROW(random_decision_vector({{1, -inf}, {0, 32}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{1, 2, 3}, {0, 3}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{0, 2, 3}, {1, 4, nan}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{0, 2, nan}, {1, 4, 4}}, r_engine), std::invalid_argument); |
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.
Maybe add tests for the infinity checks?
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.
So I think there is some confusion on how we deal with infinites.
Status quo:
- It is allowed to have infinite bounds on the problem.
- It is not allowed to draw at random a number if the bounds are infinite.
These cases are already tested on the c++ side.
With the addition of the integer part I would not change this behaviour and keep the possibility to have infinite bounds on the problem (also for integers), while preventing to draw random decision vectors from infinite bounds.
The reason for this seemingly asymmetric behaviour is that
- It makes sense to signal to the optimizer
algo.evolve()
that the problem is unbounded. - It makes little sense, if the bounds are infinite, to construct a decision vector drawing randomly from it, better to have the user explicitly init the pop in that case.
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.
Right, I forgot that the random vector stuff errors out with infinities.
@darioizzo looks good. The only real issue I have found is the one about generating random decision vectors (see comments). |
include/pagmo/utils/generic.hpp
Outdated
retval[i] = uniform_real_from_range(bounds.first[i], bounds.second[i], r_engine); | ||
} | ||
if (nix > 0u) { |
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.
Two notes:
- I think you can skip the
if (nix > 0u)
because you have already thefor
cycle checking for the same condition right? - How about using
std::nextafter
for having a closed interval. See: http://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution/uniform_real_distribution
To create a distribution over the closed interval [a,b], std::nextafter(b, std::numeric_limits::max()) may be used as the second parameter.
include/pagmo/utils/generic.hpp
Outdated
retval[i] = uniform_real_from_range(bounds.first[i], bounds.second[i], r_engine); | ||
} | ||
if (nix > 0u) { | ||
for (decltype(nx) i = ncx; i < nx; ++i) { |
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.
auto i = ncx
tests/generic.cpp
Outdated
@@ -71,16 +71,33 @@ BOOST_AUTO_TEST_CASE(random_decision_vector_test) | |||
BOOST_CHECK_THROW(random_decision_vector({{1, -inf}, {0, 32}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{1, 2, 3}, {0, 3}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{0, 2, 3}, {1, 4, nan}}, r_engine), std::invalid_argument); | |||
BOOST_CHECK_THROW(random_decision_vector({{0, 2, nan}, {1, 4, 4}}, r_engine), std::invalid_argument); |
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.
Right, I forgot that the random vector stuff errors out with infinities.
self.assertTrue(int(x[1]) != x[1]) | ||
set_global_rng_seed(42) | ||
y = random_decision_vector(lb = [1.1,2.1,-3], ub = [2.1, 3.4,5], nix = 1) | ||
self.assertTrue((x == y).all()) |
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 would consider exception translation part of the boost interface.
In reply to #112, this PR modifies the problem interface adding explicit support for MINLP type of problems. In order to do so the implemented logic is based on the following design drivers
the solution of the relaxed problem calling some NLP solver.