Add Random topology class #155

whzup · 2018-07-05T16:48:40Z

Added a new topology with random neighbours. Added documentation and
a test file for it and reworked the documentation of the
GeneralOptimizer class to incorporate all available topologies.
Simplified the fixture function for the GeneralOptimizer class.
Cited the relevant paper for the algorithm implemented.

I implemented the algorithm to compute the neighbours in a separate, private method inside the Random class because I thought it's cleaner that way. We need such a complicated algorithm because it's important that the "graph" we create when assigning neighbours is fully connected.

whzup · 2018-07-05T18:39:27Z

@ljvmiranda921 the static/dynamic feature is already in the pipeline and almost ready for PR 😄. Just gonna wait for you to approve this one so I can work from a clean branch.
Please don't feel stressed by my many PRs and take as much time as needed to review them!

ljvmiranda921 · 2018-07-06T15:10:49Z

The static/dynamic feature is already in the pipeline and almost ready for PR

Wow, you're an MVP of this version haha. Thanks a lot for all your help!

Please don't feel stressed by my many PRs and take as much time as needed to review them!

Haha! No problem. Will probably review them next week, @whzup , after my final defense (wish me luck)!

whzup · 2018-07-07T07:49:45Z

Hahaha, I thought I'm going to push out some PRs before I go work 😄. Sure do! Wish you success! Is the thesis going to be available online? Might take a glance at it if so 😋.

ljvmiranda921 · 2018-07-07T10:09:19Z

I thought I'm going to push out some PRs before I go work

Sure @whzup ! Just push the changes and I'll take care of this next week.

Is the thesis going to be available online? Might take a glance at it if so.

Of course! The link can be found here

Added a new topology with random neighbors. Added documentation and a test file for it and reworked the documentation of the GeneralOptimizer class to incorporate all available topologies. Simplified the fixture function for the GeneralOptimizer class. Cited the relevant paper for the algorithm implemented.

whzup · 2018-07-09T18:56:27Z

Just updated the documentation for the Random topology to fit the changes I made in the other topology documentations 👍.

ljvmiranda921

Overall, awesome work!

I have some comments regarding documentation (perhaps an explanation on how dijkstra plays into the Topology and some dev explanations on the adj_*, aux_* and dist_* matrices). There's also a comment on reducing the need for nested for-loops via itertools, please check it out.

As for tests, I'd recommend you test the __compute_neighbors() method so that we know it works as expected. The other tests were great! I love that you parametrized the topologies for the GeneralOptimizer.

Sorry this took too long. Final defense was done yesterday and it went well!

ljvmiranda921 · 2018-07-11T02:02:46Z

pyswarms/backend/topology/random.py

+"""
+A Random Network Topology
+
+This class implements a random topology. All particles are connected in a random fashion.


It would be nice if we add a short explanation on why dijkstra is used and how it relates to the Random topology. 😄

ljvmiranda921 · 2018-07-11T02:04:23Z

pyswarms/backend/topology/random.py

+            a Swarm instance
+        k : int
+            number of neighbors to be considered. Must be a
+            positive integer less than :code:`n_particles-1`


A bit nitpicky but an empty line before Returns will make everything consistent 👍

ljvmiranda921 · 2018-07-11T02:16:37Z

pyswarms/backend/topology/random.py

+                for j in range(swarm.n_particles):
+                    if dist_matrix[i][j] == 0:
+                        adj_matrix[i][j] = 1
+


Wanna try itertools.product for this nested for-loop? Here's the documentation. Sample usage:

import itertools for i, j in itertools.product(range(swarm.n_particles), repeat=2): # Not sure if this ternary operator works, if not, just use the normal one adj_matrix[i][j] = 1 if dist_matrix[i][j] == 0

ljvmiranda921 · 2018-07-11T02:20:28Z

pyswarms/backend/topology/random.py

+                for j in range(swarm.n_particles):
+                    if dist_matrix[i][j] == 0:
+                        adj_matrix[i][j] = 1
+


From my understanding, the elements in adj_matrix are initially set to 1. Then we have a for-loop that traverses the dist_matrix and updates the adj_matrix to 1 if the corresponding element in the dist_matrix is equal to 0.

Does this change anything? Perhaps it's connected_components that makes the difference but maybe you can explain it more clearly? 👍

I was wondering about this part as well. Only when I tested the algorithm without the condition that you test it for connectivity it became clear to me. If you don't have a connected graph it is quite possible that you can't extract the best position from the swarm. It might not be a problem with larger swarm sizes but in smaller ones, it occurred very often. I think this happens because the compute_gbest() method can't handle multiple unconnected swarm clusters. Just wanted to calrify this because it's hard to stuff all this information in a 3 line comment 😄. I'll go ahead and change these things in the evening 👍.

Just wanted to calrify this because it's hard to stuff all this information in a 3 line comment smile

Understood. Yes it does look cluttered. I suggest we put this on the docstring instead with a note markup. Just revert this back to # Generate connected graph
and put the explanation you've added as a note on __compute_neighbors()'s docstring. 👍

So what I'm thinking is that inside the __compute_neighbors()'s docstring, we have an explanation for all three matrices:

def __compute_neighbors(): """Single-line comment etc. etc. - adj_matrix: does this and this and contains this - dist_matrix: contains this, relates to this, etc. - aux_matrix: performs this, etc. """

ljvmiranda921 · 2018-07-11T02:24:59Z

pyswarms/backend/topology/random.py

+        This method computes the adjacency matrix of the topology using
+        the randomized algorithm proposed in [TSWJ2013]. The resulting
+        topology is a connected graph.
+


I think we can add a short explanation between the adj_matrix, aux_matrix, and dist_matrix here in order to guide other developers who may someday work with this code. 👍

ljvmiranda921 · 2018-07-11T02:27:12Z

tests/backend/topology/test_random.py

+
+# Import from package
+from pyswarms.backend.topology import Random
+


Please add a test for __compute_neighbors 👍
This seems to be an integral part of the topology and it would be nice if we can make sure that it will always work as expected. 💯

You can construct a "smaller" swarm in conftest.py to make it more manageable.

What do you think is the best way to test it? The elements of the output matrix are random so we can't check if it returns the "right" elements. The only things we could test are the shape and the symmetry. We could use small swarms but then it would just be lucky if we get the right answers or not.

Implemented a test for the shape and the symmetry, I hope it is sufficient for you 🙈.

I really wonder why my earlier reviews didn't show up 😕 So you can probably try to set a random seed (not sure if we set the numpy random seed or the standard library's random seed) to some value so that it will always return the correct values whenever the __compute_neighbors() is called.

So yes, just check if setting a random seed will work so that we always have an expected behaviour

ljvmiranda921 · 2018-07-11T02:28:28Z

tests/optimizers/conftest.py

+                    Random()
+                ])
+def general_opt_reset(top):
+    """Returns a GeneralOptimizerPSO instance that has been run and reset to check
    default value"""


Awesome! 🥇

ljvmiranda921 · 2018-07-11T02:30:48Z

tests/optimizers/test_general_optimizer.py

@@ -52,6 +67,21 @@ def test_invalid_k_or_p_values(options):
        GeneralOptimizerPSO(5, 2, options, Ring())




Great tests! To be honest, the GeneralOptimizer can make everything DRY. The reason we will still keep the GlobalBest and LocalBest is for tradition— they're the commonly-known PSO algorithms, and having them exposed to a first-time user is good.

I agree. They're also very easy to handle if you want to make a quick optimization. The GeneralOptimizer class will surely grow and so might the complexity of its handling. I guess it's a good decision to leave the others in for simplicity's sake.

whzup · 2018-07-11T17:27:52Z

@ljvmiranda921, nice! Thanks for the good inputs! Surely went well the thesis looks pretty good 👍. Done with all changes except the testing for the compute_neighbors() method. Not sure how to test it, TBH😄.

ljvmiranda921 · 2018-07-12T09:06:48Z

Thanks a lot @whzup ! This PR looks good and nicely-structured.

Thanks for the good inputs! Surely went well the thesis looks pretty good +1.

Thank you so much! I appreciate that you liked it!

Done with all changes except the testing for the compute_neighbors() method. Not sure how to test it, TBH

So my idea is that you contrive a small swarm from the Swarm class and add it to conftest.py (or you can use the existing swarm fixture), then, try to pass it to __compute_neighbors and see the return value. That will then be your expected value.

After that, we write a test that checks if it will always give us the expected value whenever the test suite is run. If there is stochasticity (randomness) involved, you can set the random seed (not sure if you will set numpy's random seed or the random library's seed) so that it will always return the same thing. 👍

whzup · 2018-07-12T17:55:05Z

Done! Hope you like it 😄. I'm already itching to PR the static/dynamic feature 😋.

ljvmiranda921

Woops let me check. I wonder why my previous review comments didn't show up...

ljvmiranda921 · 2018-07-12T08:57:08Z

pyswarms/backend/topology/random.py

+                for j in range(swarm.n_particles):
+                    if dist_matrix[i][j] == 0:
+                        adj_matrix[i][j] = 1
+


Just wanted to calrify this because it's hard to stuff all this information in a 3 line comment smile

Understood. Yes it does look cluttered. I suggest we put this on the docstring instead with a note markup. Just revert this back to # Generate connected graph
and put the explanation you've added as a note on __compute_neighbors()'s docstring. 👍

ljvmiranda921 · 2018-07-12T08:59:20Z

pyswarms/backend/topology/random.py

+                for j in range(swarm.n_particles):
+                    if dist_matrix[i][j] == 0:
+                        adj_matrix[i][j] = 1
+


So what I'm thinking is that inside the __compute_neighbors()'s docstring, we have an explanation for all three matrices:

def __compute_neighbors(): """Single-line comment etc. etc. - adj_matrix: does this and this and contains this - dist_matrix: contains this, relates to this, etc. - aux_matrix: performs this, etc. """

ljvmiranda921

Thanks for this @whzup ! Just a few short things to edit (and 1 more test) so that I can merge everything by this week. Very excited to see the static implementations soon!

Less clutter in the code. It would be nice to move all long explanations into the docstring. The idea is to have short explanations in the inline-comments, and have more verbose descriptions in the docstring. I suggest you also put something like:

adj_matrix: does this etc. etc. etc.
aux_matrix: creates this etc.
dist_matrix: contains this etc.

So that everything is manageable and more readable without looking into the code.

Set random seed so we can have an expected behaviour. For the tests, try setting a random seed so that we will always have an expected behaviour whenever __compute_neighbors() is called. Just contrive a very small swarm (or you can use the preset ones in conftest), and call the required method.

ljvmiranda921 · 2018-07-13T02:36:08Z

pyswarms/backend/topology/random.py

+        # best position within the swarm. If the element (i,j) of the
+        # distance matrix is 0, that means that there is no path
+        # from node i to node j, thus there are still unconnected
+        # pieces of the graph (i.e. topology)


Hi @whzup , one last thing regarding documentation. I'm not sure if you've seen my earlier comment but I suggested to put this long text on the docstring itself. You can just put a # Generate connected graph on the docstring. The idea is to make the docstring a bit longer while the in-line comments shorter (less clutter) and can be viewed by the developer using help() without resorting to look at the source code.

ljvmiranda921 · 2018-07-13T02:37:43Z

tests/backend/topology/test_random.py

+
+# Import from package
+from pyswarms.backend.topology import Random
+


I really wonder why my earlier reviews didn't show up 😕 So you can probably try to set a random seed (not sure if we set the numpy random seed or the standard library's random seed) to some value so that it will always return the correct values whenever the __compute_neighbors() is called.

ljvmiranda921 · 2018-07-13T02:41:20Z

tests/backend/topology/test_random.py

+
+# Import from package
+from pyswarms.backend.topology import Random
+


So yes, just check if setting a random seed will work so that we always have an expected behaviour

Updated the documentation of the Random class. Especially the __compute_neighbor() method. Added the comments inside the method to the docstring and deleted irrelevant comments. Changed the nested for-loops to one loop with the itertools library. Added a new test for the return value of the __compute_neighbor() method, which checks the shape and the symmetry. Added a new test for the return value of the __compute_neighbors() method, which compares the returned matrix with a preset comparison matrix using a seed.

ljvmiranda921

LGTM! Will merge in a bit

ljvmiranda921 · 2018-07-14T04:00:00Z

pyswarms/backend/topology/random.py

+            * dist_matrix : The distance matrix computed with Dijkstra's
+                            algorithm. It is used to determine where the
+                            graph needs edges to change it to a connected
+                            graph.


Awesome! This is good @whzup !

whzup added enhancement Feature requests v0.3.0 labels Jul 6, 2018

ljvmiranda921 self-requested a review July 6, 2018 15:12

ljvmiranda921 assigned ljvmiranda921 and whzup Jul 6, 2018

ljvmiranda921 requested changes Jul 11, 2018

View reviewed changes

ljvmiranda921 approved these changes Jul 13, 2018

View reviewed changes

ljvmiranda921 reviewed Jul 13, 2018

View reviewed changes

ljvmiranda921 requested changes Jul 13, 2018

View reviewed changes

ljvmiranda921 approved these changes Jul 14, 2018

View reviewed changes

ljvmiranda921 merged commit cca8d3c into ljvmiranda921:development Jul 14, 2018

whzup deleted the random_topology branch July 14, 2018 05:22

ljvmiranda921 mentioned this pull request Jul 31, 2018

Prepare v.0.3.0 release #180

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Random topology class #155

Add Random topology class #155

whzup commented Jul 5, 2018

whzup commented Jul 5, 2018

ljvmiranda921 commented Jul 6, 2018 •

edited

Loading

whzup commented Jul 7, 2018

ljvmiranda921 commented Jul 7, 2018 •

edited

Loading

whzup commented Jul 9, 2018 •

edited

Loading

ljvmiranda921 left a comment

ljvmiranda921 Jul 11, 2018

ljvmiranda921 Jul 11, 2018

ljvmiranda921 Jul 11, 2018

ljvmiranda921 Jul 11, 2018

whzup Jul 11, 2018

ljvmiranda921 Jul 12, 2018

ljvmiranda921 Jul 12, 2018

ljvmiranda921 Jul 11, 2018

ljvmiranda921 Jul 11, 2018

whzup Jul 11, 2018

whzup Jul 11, 2018

ljvmiranda921 Jul 13, 2018

ljvmiranda921 Jul 13, 2018

ljvmiranda921 Jul 11, 2018

ljvmiranda921 Jul 11, 2018

whzup Jul 11, 2018

whzup commented Jul 11, 2018

ljvmiranda921 commented Jul 12, 2018 •

edited

Loading

whzup commented Jul 12, 2018 •

edited

Loading

ljvmiranda921 left a comment •

edited

Loading

ljvmiranda921 Jul 12, 2018

ljvmiranda921 Jul 12, 2018

ljvmiranda921 left a comment •

edited

Loading

ljvmiranda921 Jul 13, 2018

ljvmiranda921 Jul 13, 2018

ljvmiranda921 Jul 13, 2018

ljvmiranda921 left a comment

ljvmiranda921 Jul 14, 2018


		# Import from package
		from pyswarms.backend.topology import Random

		@@ -52,6 +67,21 @@ def test_invalid_k_or_p_values(options):
		GeneralOptimizerPSO(5, 2, options, Ring())

Add Random topology class #155

Add Random topology class #155

Conversation

whzup commented Jul 5, 2018

whzup commented Jul 5, 2018

ljvmiranda921 commented Jul 6, 2018 • edited Loading

whzup commented Jul 7, 2018

ljvmiranda921 commented Jul 7, 2018 • edited Loading

whzup commented Jul 9, 2018 • edited Loading

ljvmiranda921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whzup commented Jul 11, 2018

ljvmiranda921 commented Jul 12, 2018 • edited Loading

whzup commented Jul 12, 2018 • edited Loading

ljvmiranda921 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljvmiranda921 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljvmiranda921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljvmiranda921 commented Jul 6, 2018 •

edited

Loading

ljvmiranda921 commented Jul 7, 2018 •

edited

Loading

whzup commented Jul 9, 2018 •

edited

Loading

ljvmiranda921 commented Jul 12, 2018 •

edited

Loading

whzup commented Jul 12, 2018 •

edited

Loading

ljvmiranda921 left a comment •

edited

Loading

ljvmiranda921 left a comment •

edited

Loading