particle_swarm #218

abieler · 2016-06-09T16:55:12Z

implementation of the adaptive particle swarm algorithm which is a global optimization method.

tested this algorithm on many real world problems. although being slower than
the gradient based versions it proves stable, especially for systems with "real" data
which can be noisy or in other ways hampered.

I tried my best to have the code in similar form as the other packages in Optim.jl, but surely there
is a lot of room for improvement.

No optimization for speed done yet.

My first PR, so go easy on my ;)

pkofod · 2016-06-09T17:03:53Z

I'll review it as soon as I can. Do you have a reference?

johnmyleswhite · 2016-06-09T17:04:35Z

This is awesome. I'll make a cursory review and let @pkofod handle the rest.

johnmyleswhite · 2016-06-09T17:07:30Z

src/particle_swarm.jl

@@ -0,0 +1,383 @@
+using Distributions


You generally shouldn't do this in library code for several reasons:

You've added a dependency that was intentionally removed from this package in the past.

You've done it in the wrong place: dependencies should always be listed in the main package file at src/Optim.jl because they have global effects

You've introduced a major change in the global namespace.

My suggestions:

Try to find a way to not use Distributions.jl. I won't say that's an absolute requirement for merging, but think of it as being one to motivate you. :)

Make sure you move the importing to src/Optim.jl.

Do the importing using import Distributions and use fully qualified names like Distributions.Gamma everywhere.

abieler · 2016-06-09T17:09:00Z

here is the reference to the paper
Zhan et al. 2009. IEEE TRANSACTIONS ON SYSTEMS, MAN, AND CYBERNETICS—PART B: CYBERNETICS, VOL. 39, NO. 6, DECEMBER 2009

johnmyleswhite · 2016-06-09T17:09:14Z

src/particle_swarm.jl

+immutable ParticleSwarm <: Optimizer
+  xmin::Array
+  xmax::Array
+  nParticles::Int


This is the wrong case convention for Julia code: you should use underscores here.

johnmyleswhite · 2016-06-09T17:13:11Z

This is a very impressive first PR. I've made a lot of comments about stylistic conventions you should try to adopt when contributing to packages with an existing style of code. I'll let Patrick make sure the substantive stuff is right.

abieler · 2016-06-09T17:18:36Z

Thanks for the advice!
I ll need some help on the whole trace thing, but I ll look into it myself some more and try getting a hang of it.

pkofod · 2016-06-09T18:01:10Z

src/optimize.jl

+        autodiff = autodiff)
+    method = get_optimizer(method)::Optimizer
+    optimize(f, initial_x, method, options)
+end


What is your intention here? You're basically creating a new method that defaults to ParticleSwarm if given a function and an array, but the method just below overrides that method, and NelderMead is again the default (which it probably should be, unless we find particle swarm to be a better default solver).

my bad. I simply copied from NelderMead() and forgot to get rid of it after the fact.

pkofod · 2016-06-09T18:17:09Z

I'm in the middle of moving these days, so I don't have too much time, but I will get back to this. I will also read the article just to have two set of eyes that can catch bugs.

Generally, I think you should start with John Myles White's comments, as I agree with those. Then I'll take a shot at giving comments when that is in place. What is the problem with the traces?

codecov-io · 2016-06-10T10:24:07Z

Current coverage is 84.47%

Merging #218 into master will increase coverage by 0.29%

@@             master       #218   diff @@
==========================================
  Files            30         31     +1   
  Lines          1896       2138   +242   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1596       1806   +210   
- Misses          300        332    +32   
  Partials          0          0

Powered by Codecov. Last updated by 21bd318...83a745f

abieler · 2016-06-12T17:17:12Z

sorry guys but I need some git help to proceed here.
I want to continue working on this PR on a different machine than when I started.
So I did:

git fetch origin pull/218/head:particle_swarm
git checkout particle_swarm
<change code>
git add <stuff>
git commit

but now git tells me I cannot push my changes
(git config --global push.default simple)
with the following message:

fatal: The current branch particle_swarm has no upstream branch.
To push the current branch and set the remote as upstream, use

    git push --set-upstream origin particle_swarm

Is the suggested step from the error message the right way to proceed? I dont want to mess up stuff.. :)

johnmyleswhite · 2016-06-12T17:20:38Z

Honestly, you're best off just making a copy of any work that's unsaved and trying things until they work. Something like the suggestion in the error message is likely to work, but I'm not sure exactly what's necessary.

abieler · 2016-06-12T17:27:37Z

well.. :) what would be the proper way to start something like this? (this = working on a PR from a different machine)

johnmyleswhite · 2016-06-12T17:30:42Z

Ignore the PR part. You have a repo (which is your fork) and a branch (which is the branch you're using on your fork). You need to clone your repo, checkout the branch and do work from there.

abieler · 2016-06-12T17:34:52Z

Yes, that is what I was thinking initially (and will do this from now on) but the reason I went the other way was this thought: How can I then pull the latest changes anyone else does on the original Optim.jl code into my repo?

abieler · 2016-06-12T17:37:01Z

Oh and the following: When cloning my repo to my machine: where do I clone it to? If it is a arbitrary location, how do I tell Julia to use that one? Sorry for the noobery...

pkofod · 2016-06-20T07:30:33Z

I'm not sure I follow, didn't you just say n equal to length of x was a good rule of thumb ? At least n equal to 10 seems too small for some of the problems.

abieler · 2016-06-20T08:45:12Z

True, but because lenght(x) < 100 there will not be an increase in
performance compared to the runs with 100 particles. At least in terms of
the quality of the solutions, the runtime will of course decrease. So rule
of thumb i d say n_particles >= length(x) with more particles the better if
you dont care about computational cost.
On Jun 20, 2016 9:30 AM, "Patrick Kofod Mogensen" [email protected]
wrote:

I'm not sure I follow, didn't you just say n equal to length of x was a
good rule of thumb ? At least n equal to 10 seems too small for some of the
problems.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#218 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGwUhZzA9t2ZFLW_vE3irHc7HJSLr0GFks5qNkGagaJpZM4IyL41
.

pkofod · 2016-06-20T08:52:08Z

Sure, I just wanted to see how it performed given that particular rule of thumb. I didn't expect it to outperform n=100

pkofod · 2016-06-22T13:58:11Z

Good catch on the limit_search_space being superfluous as a field in ParticleSwarm. :) I missed that one.

Edit: I'm not sure the const is necessary though. I'm not sure it does what you think. Consider

julia> function f(x)
       const y = 3
       y = x
       y
       end
f (generic function with 1 method)

julia> f(34)
34

pkofod · 2016-07-03T19:30:05Z

@abieler have you given the const remark any thought? :)

abieler · 2016-07-03T20:09:04Z

To be honest, no.. Was buried under a pile of work and have not looked at this for a while now.
You suggest removing all the const definitions? I can do this quickly if you confirm and blindly trust
your judgement :)

pkofod · 2016-07-03T20:09:55Z

To be honest, no.. Was buried under a pile of work and have not looked at this for a while now.
You suggest removing all the const definitions? I can do this quickly if you confirm and blindly trust
your judgement :)

You should never trust my judgement :) I just don't think they add anything, but I could be wrong. Edit Also, if you're busy, don't worry. There's no rush!

abieler · 2016-07-03T20:14:17Z

I ll free some time next week to look into this.

pkofod · 2016-07-03T20:35:28Z

I ll free some time next week to look into this.

Cool, let me know if I can help you.

abieler · 2016-07-07T09:37:42Z

profiling with and without const showed no difference in performance, so I removed them.

abieler · 2016-07-07T09:42:01Z

there is still the issue with my arbitrary choices of

f_converged = true
ftol = 1e-2

which are hard coded after an optimization run.
f_converged makes little sense in this algorithm as it can always reset and jump out of a local minimum.
(as long as maxIteration is not reached)

same thing for ftol. I have no use for this variable (yet?) in the current implementation.

pkofod · 2016-07-07T12:00:12Z

Just return false and nan?

abieler · 2016-07-07T12:07:11Z

Can do. Was more of a question on what makes sense or if there is a convention of some sorts for similar cases.

abieler · 2016-07-07T20:58:16Z

If there is some small thing left to do I can probably take care of it tomorrow. I ll then be off the grid for 3 weeks starting Saturday.

pkofod · 2016-07-08T09:49:29Z

I will see if I can find something, but right now I think it's pretty good to go. If there are minor details, I can fix them after a merge. It would be nice if you could add a bit of details to the docs (when you return) about the implementation, and how to control the various parts of the algorithm.

abieler · 2016-07-08T11:08:51Z

Cool. Looking forward to being a contributor to Julia :D
I ll add some documentation for sure.

I also played a little bit with an option of a swarm with limited memory. this could be used in cases
where the optimum is moving/drifting over time. Happened in my case due to temperature effects on the sensor I was optimizing parameters for.

One step at a time.
Thanks for all the support so far. Learned a lot and had fun too :)

pkofod · 2016-07-08T11:38:19Z

I'll look it through one more time later, and then I will probably merge unless someone else has any comments.

And thank you for the contribution.

bjarthur · 2016-07-13T13:50:08Z

rebase the commits into just one perhaps?

pkofod · 2016-07-13T15:02:28Z

Github allows you to squash upon merge now, so I'll just do that :)

pkofod · 2016-07-13T22:29:15Z

src/particle_swarm.jl

+    n_particles::Int
+end
+
+ParticleSwarm() = ParticleSwarm([], [], 0)


I hadn't seen these. This should probably just be a empty constructor with keywords. I might just merge and fix it.

pkofod · 2016-07-13T22:33:29Z

Work left to be done here : change xmin and xmax to x_min and x_max, fix the constructor, and add docs.

I will merge it tomorrow, and fix the notation thing, and the constructor in a follow-up commit, as @abieler is off the grid. It would be wonderful if you could add docs when you get back. I didn't think about it earlier, but I also asked for it in the #238 PR. I think it is much better if people who contribute a solver also has a go at the docs themselves.

pkofod · 2016-07-14T22:10:15Z

A great first PR! Thanks for doing this.

abieler added 5 commits June 9, 2016 09:36

initial commit

8a4bad2

adds ParticleSwarm

659511d

adds particle_swarm()

82d95e2

clean up

8f9488e

adds tests for particle_swarm algorithm

8f5152c

johnmyleswhite reviewed Jun 9, 2016
View reviewed changes

pkofod reviewed Jun 9, 2016
View reviewed changes

fixes code style issues pointed out by John.

0209f36

abieler changed the title ~~Pull request/8f5152c4~~ particle_swarm Jun 12, 2016

pkofod force-pushed the master branch from e747f3f to 21bd318 Compare June 21, 2016 15:37

abieler added 3 commits June 22, 2016 07:36

adds default value for empty ParticleSwarm() constructor.

75ea0ae

more convenience constructors

c9f7222

removes optimize() definition that defaults to ParticleSwarm

0789eb5

pkofod mentioned this pull request Jun 30, 2016

added bounds argument to levenberg_marquardt #196

Merged

removes const

72d203b

setting converged=false and ftol=NaN

83a745f

pkofod reviewed Jul 13, 2016
View reviewed changes

pkofod merged commit a3cc76e into JuliaNLSolvers:master Jul 14, 2016

particle_swarm #218

particle_swarm #218

Conversation

abieler commented Jun 9, 2016

pkofod commented Jun 9, 2016

johnmyleswhite commented Jun 9, 2016

johnmyleswhite Jun 9, 2016

Choose a reason for hiding this comment

abieler commented Jun 9, 2016

johnmyleswhite Jun 9, 2016

Choose a reason for hiding this comment

johnmyleswhite commented Jun 9, 2016

abieler commented Jun 9, 2016

pkofod Jun 9, 2016

Choose a reason for hiding this comment

abieler Jun 9, 2016

Choose a reason for hiding this comment

pkofod commented Jun 9, 2016

codecov-io commented Jun 10, 2016 • edited Loading

Current coverage is 84.47%

abieler commented Jun 12, 2016

johnmyleswhite commented Jun 12, 2016

abieler commented Jun 12, 2016

johnmyleswhite commented Jun 12, 2016

abieler commented Jun 12, 2016

abieler commented Jun 12, 2016

pkofod commented Jun 20, 2016

abieler commented Jun 20, 2016

pkofod commented Jun 20, 2016

pkofod commented Jun 22, 2016 • edited Loading

pkofod commented Jul 3, 2016

abieler commented Jul 3, 2016

pkofod commented Jul 3, 2016 • edited Loading

abieler commented Jul 3, 2016

pkofod commented Jul 3, 2016

abieler commented Jul 7, 2016

abieler commented Jul 7, 2016

pkofod commented Jul 7, 2016

abieler commented Jul 7, 2016

abieler commented Jul 7, 2016

pkofod commented Jul 8, 2016

abieler commented Jul 8, 2016

pkofod commented Jul 8, 2016

bjarthur commented Jul 13, 2016

pkofod commented Jul 13, 2016

pkofod Jul 13, 2016

Choose a reason for hiding this comment

pkofod commented Jul 13, 2016

pkofod commented Jul 14, 2016

codecov-io commented Jun 10, 2016 •

edited

Loading

pkofod commented Jun 22, 2016 •

edited

Loading

pkofod commented Jul 3, 2016 •

edited

Loading