Boundary Constraint Handling Mechanisms by KyleErwin · Pull Request #274 · ciren/cilib

KyleErwin · 2018-03-28T14:42:47Z

Added a few bchms that I did in a project. Some of these were given by prof, and others I found in reading.
There are few others that I did that involved in re-initialization with randomness that I just want to rework to be neater.

gpampara · 2018-03-28T14:45:40Z

Is there a reason why the functions are reevaluating the entity? Additionally, this need not be restricted to Particle, the implicit will ensure that there is the needed state, so it should just be any Entity

gpampara

There is a bunch of code that seems to be duplicated for the bounds checking, I'd prefer if we extract that to a function and then pass a function in that does the "different" actions.

gpampara · 2018-03-28T14:48:01Z

    if (b) updatePBest(p) else Step.point(p)
  }

+  def clamping[S](particle: Particle[S, Double])(


This is too specific. We could do better here by ensuring that we don't specialize to Double, instead it would be better if we add a type-param and ensure that there is an Order instance instead so that the comparisons can be done, without caring what the type really is.

gpampara · 2018-03-28T14:50:11Z

+  }
+
+  def initToPBAndZeroedVelocity[S](particle: Particle[S, Double])(
+    implicit M: HasMemory[S, Double], V: HasVelocity[S, Double]): Step[Double, Particle[S, Double]] = {


It seems that composition would be the better strategy here -> could we maybe try see if we can define the zeroing separately and then compose them to get the desired result?

gpampara · 2018-03-28T14:50:47Z

+      val newVelocity = updated.map(t => t._2)
+
+      for {
+          v <- PSO.updateVelocity(particle, Position.apply(newVelocity, particle.pos.boundary))


We should really use a Lens to do this update instead of recreating the position

gpampara · 2018-03-28T14:51:37Z

+      } yield u
+  }
+
+  def initToGB[S](particle: Particle[S, Double], gBest: Position[Double])(


Let's see if we can generalize this to only need an Order, instead of using Double

KyleErwin · 2018-03-28T14:59:34Z

Okay cool. I'll change it to entity.
The reason they are reevaluated was because a particle's position and/or velocity are being modified. And when they are modified their fitness values become invalid. I was using a Gbest pso, and these bchms were placed at end of the my pso for step compression. Without the eval step there werent any valid fitness values to be used in the next iteration. In hindsight I perhaps should have placed the bchm before the eval step to avoid this reevaluating thing..

KyleErwin · 2018-03-29T06:06:22Z

@gpampara Ive made the code more generic and simpler. Let me know what you think

gpampara

This is looking much nicer!

The boundary functions are now no longer related to the PSO / Particle, and should probably be moved out of the pso package and perhaps into Entity? I'm not sure where a good home would be for them. I'm quite pleased that they are in the shape that they are in currently though.

Looking at the code, there is a common pattern that I see emerging. The Position of the entity is modified and then the _vector is then set, with the way that the new vector is constructed being the only real change. I wonder if we should try extract this into a general function, passing in the construction as a function parameter?

gpampara · 2018-03-29T06:56:58Z

+
+  def initToGB[S,A:scalaz.Equal](x: Entity[S,A], gBest: Position[A])(implicit N: spire.math.Numeric[A]) = 
+    Lenses._position.modify((p: Position[A]) => {
+      val c: NonEmptyList[A] = p.pos zip p.boundary zip gBest.pos map {x => flatten(x)} map {


The one thing bothering me with this code is that it is partial - this is actually true for all the functions that use the case to match the different options.

The poly function should probably be removed, and we can replace the double map with a single one. For example:

val c: NonEmptyList[A] = p.pos zip p.boundary zip gBest.pos map { case ((p, b), g) => if (b.contains(N.toDouble(p))) p else g }

The case in the above is only there to destructure the tuple and is unfortunately a requirement in this usage because the scala compiler is a bit lacking wrt expressive power.

gpampara · 2018-03-29T06:57:33Z

 object PSO {
  import Lenses._

+  trait LowPriorityFlatten extends Poly1 {


This should probably be removed.

gpampara · 2018-04-04T10:20:30Z

@KyleErwin Please run the code through the scalafmt formater. Can just run scalafmt from sbt.

KyleErwin · 2018-04-04T10:31:12Z

@gpampara formatted the code

gpampara · 2018-04-04T10:32:34Z

        case t if N.toDouble(t._1) < t._2.lowerValue => N.fromDouble(t._2.lowerValue)
        case t if N.toDouble(t._1) > t._2.upperValue => N.fromDouble(t._2.upperValue)
-        case t if t._2.contains(N.toDouble(t._1)) => t._1
+        case t if t._2.contains(N.toDouble(t._1))    => t._1


So, what would be the behavior if t._1 is not contained in t._2? However unlikely that should be? I'm curious as this is a partial function, and they usually bite you later.

Hmm I see what you're saying. I'll put the first two cases, the greater than and less than cases, in to a parent case that is a doesNotContain. If that makes sense?

KyleErwin · 2018-04-04T13:49:37Z

@gpampara I think I have removed all partial functions. In the partial cases it was simpler to convert them to if-else blocks

gpampara

It's looking much better!

I think we should adjust the remainder of the functions to also be if-expression based instead of the case statements. They all follow a similar format and the expression means that we are catering for one or another case only.

gpampara · 2018-04-04T14:05:58Z

+  def clamp[S, A: scalaz.Equal](x: Entity[S, A])(implicit N: spire.math.Numeric[A]) =
+    Lenses._position.modify((p: Position[A]) => {
+      val c: NonEmptyList[A] = p.pos.zip(p.boundary).map {
+        case (p, b) => {


You can lose the curly braces here, seeing as the expression is only an if and always has a resulting value. This if based version is much better imho!

What I means is:

case (p, b) => if ....

gpampara · 2018-04-04T14:07:30Z

+                                                    M: HasMemory[S, A]) =
+    Lenses._position.modify((p: Position[A]) => {
+      val c: NonEmptyList[A] = p.pos.zip(p.boundary).zip(M._memory.get(x.state).pos).map {
+        case ((p, b), _) if b.contains(N.toDouble(p))       => p


The same argument about translating this to a simple if holds here as well.

gpampara · 2018-04-05T10:34:07Z

Looking great. Will merge a little later today

gpampara · 2018-04-06T07:00:44Z

After some more thought, we can do better :)

I'll make some notes of what I'm thinking - a lot of the functions are transformations that are not really involved with Lenses.

gpampara · 2018-05-13T15:29:01Z

@KyleErwin Please have a look at #283, if you're happy with that approach, could you close this PR?

Added a few bchms

4db401c

gpampara requested changes Mar 28, 2018

View reviewed changes

KyleErwin added 3 commits March 29, 2018 08:00

Generic methods

7ac6b7a

Formating

ff2212c

flatten for tuples

48c6afe

gpampara requested changes Mar 29, 2018

View reviewed changes

KyleErwin added 2 commits April 2, 2018 17:21

Moved to a nchm object in Entity

0d2ed88

Removed tuple flatten

fb174ae

scalafmt

084c8b1

Merge branch 'master' into BCHMs

2b5cbcd

gpampara reviewed Apr 4, 2018

View reviewed changes

KyleErwin added 2 commits April 4, 2018 15:22

No partial functions

632a121

Merge remote-tracking branch 'origin/BCHMs' into BCHMs

634faf2

gpampara requested changes Apr 4, 2018

View reviewed changes

KyleErwin added 3 commits April 4, 2018 17:15

if statements

1a80d11

Merge remote-tracking branch 'upstream/master' into BCHMs

8356829

Merge remote-tracking branch 'origin/BCHMs' into BCHMs

8e70bf8

KyleErwin closed this May 13, 2018

Conversation

KyleErwin commented Mar 28, 2018

Uh oh!

gpampara commented Mar 28, 2018

Uh oh!

gpampara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KyleErwin commented Mar 28, 2018

Uh oh!

KyleErwin commented Mar 29, 2018

Uh oh!

gpampara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpampara commented Apr 4, 2018

Uh oh!

KyleErwin commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KyleErwin commented Apr 4, 2018

Uh oh!

gpampara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpampara commented Apr 5, 2018

Uh oh!

gpampara commented Apr 6, 2018

Uh oh!

gpampara commented May 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants