-
Notifications
You must be signed in to change notification settings - Fork 123
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
Introducing LBest PSO for Unconstrained Optimization Problems #86
Conversation
Need to get commits fixing SPSA parameters.
localBestIndices.imbue([&]() { return index++; }); | ||
|
||
// Set sizes r1 and r2. | ||
r1.set_size(iterate.n_rows, iterate.n_cols); |
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 think we can remove the two lines here and use arma::randu(particleVelocities.n_rows, particleVelocities.n_cols
in the Update
step.
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.
Take a look at this one more time in the code, @zoq .
r1
and r2
are random vectors, and these need to change in every iteration.
In lines 127 and 128, we have r1.randu();
and r2.randu();
. Thus, in every update iteration, r1
and r2
will have different values, as is necessary in the PSO algorithm.
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.
Let me know what you think.
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.
We just change the expression in the update step from:
r1.randu();
to
arma::randu(particleVelocities.n_rows, particleVelocities.n_cols);
it would be the same, just that we don't have need to set the size before. If you prefer the current solution that's fine with me 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.
Okay we can do this.
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 think that if we follow the previous solution, the code will not have to reinitialize the variables r1
and r2
, but would only have to fill them with new random numbers. That's why I prefer the previous one.
This reverts commit b9ac5c0.
@zoq - If you could look at my documentation style in |
Okay, I've done the needful. I think this is a fairly stable implementation. In the latest push, I've implemented the lookback-horizon using Commenting throughout the code is left to be done. I'll start working on that. Besides anything regarding the commenting, let me know if you would like anything more implemented in this version. |
upperBound.fill(50); | ||
|
||
LBestPSO s(15000, lowerBound, upperBound, 3000, 200, 1e-20, 1.5, 3.0); | ||
arma::mat coordinates = arma::mat("0; 10"); |
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.
Perhaps it makes sense to use an initial starting point that is closer to the solution.
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.
That's the one thing I did not want to do. I wanted to test PSO for its robustness with non-convex problems. As a dynamical system, I'm not sure if selecting a closer initial point in a problem this hard will make much of a difference. Regardless, let me try this out. If it works, then we'll have a new observation. Also, let me try to initialize the solutions over a larger area that encompasses the solution. That's one thing that we haven't tried.
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.
Yeah, ideally we can stabilise the travis build, without bumping the number of iterations.
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've been trying for the past two hours with many different values. Can't seem to land a 100% success rate. If you're okay with it, may we exclude it for the time being?
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.
Sure, let's comment the test for now.
Before we can merge this, can you update https://github.com/mlpack/ensmallen/blob/master/HISTORY.md and add PSO to https://github.com/mlpack/ensmallen/blob/master/doc/optimizers.md. |
Yes, I will do that gladly! :) |
Looks like there is a swap file ("include/ensmallen_bits/pso/.pso.hpp.swp") that we should remove from the PR. |
This looks good, will run multiple test iterations, to make sure the pass rate is reasonable. |
Okay. I have already tested it multiple times, but take your time to test it!! |
Always good to test it on multiple machines, with different seeds each time. But haven't seen a single failure in 100+ runs. |
include/ensmallen_bits/pso/pso.hpp
Outdated
* @param explorationFactor Influence of the neighbours of the particle. | ||
*/ | ||
PSOType(const size_t numParticles = 64, | ||
arma::vec lowerBound = arma::ones<arma::vec>(1), |
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.
Just noted, we should probably use arma::vec& lowerBound
same for the upperBound
parameter, sorry missed this.
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.
Yeah, that makes sense. I'm sorry for missing this, too.
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.
There's one problem - if we use arma::vec& lowerBound
, then we cannot use a default value for lowerBound
and upperBound
. Let me know what you think would be the right thing to do.
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.
const arma::vec& lowerBound = arma::ones<arma::vec>(1)
should work.
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.
Thanks for the pointers. I've done this!
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.
This looks good to me, there are some minor style issue, that I can fix during the merge process.
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.
Second approval provided automatically after 24 hours. 👍
Thanks for a great contribution, excited to use this in practice. |
I have included code that implements LBest PSO. This code is from the fork of mlpack owned by chintans111 on GitHub that I made compatible in ensmallen. I've included tests from his fork of mlpack as well.