Created class for pyramid topology #142

whzup · 2018-06-23T09:03:30Z

Created a class for the implementation of a pyramid topology using Delaunay triangulation. Also created a test file which does not work yet! (see issue #129 for a description of the problem)
Additionally, I changed a small error in the description of the compute_velocity() method in the Ring class, where the use case imported the Star class instead of the Ring class.

Created a class for the implementation of a pyramid topology using Delaunay triangulation. Also created a test file which does *not* work yet! Additionally, I changed a small error in the description of the `compute_velocity()` method in the Ring class

ljvmiranda921 · 2018-06-23T09:37:48Z

pyswarms/backend/topology/pyramid.py

+        else:
+            return (best_pos, best_cost)
+
+        def compute_velocity(self, swarm, clamp=None):


Hey @whzup , can you check the indentation here? I think you should move this 4 spaces backward 👍
Right now, it is aligned inside def compute_gbest(), the three methods should be on the same level.
On my end, I wonder why it didn't raise any NotImplementedError...

ljvmiranda921 · 2018-06-23T09:40:05Z

pyswarms/backend/topology/pyramid.py

+            """
+            return ops.compute_velocity(swarm, clamp)
+
+        def compute_position(self, swarm, bounds=None):


The same issue with the indentation here 👍

ljvmiranda921 · 2018-06-23T09:42:42Z

Hey @whzup ! Awesome PR and I really appreciate you stepping up. For now, I think the issue with the missing argument is on the indentation of compute_position() and compute_velocity(). As of now they are aligned inside compute_gbest().

Hope this change will pass the tests from now on. Hopefully on Monday I can review the implementation itself.

Another thing, we would appreciate if you can format your git commit messages similar to (this)[https://chris.beams.io/posts/git-commit/]. I know it's weird that we use present tenses than past tenses, but yeahp, let's just follow what been done before 👍 .

Thanks a lot!

Fix indentation error in the Pyramid class that prevented the tests from running. See also: #142

whzup · 2018-06-23T12:03:02Z

Thanks for the feedback! That was actually the error and it passes the test now. I am going to test it a bit further such that it might be used in an optimization class because, at the moment, it only tests the case when it actually does the same job as the Star topology (when there are less than 5 particles in the swarm).

I'm sorry I wasn't aware of such a style guide, I'll try to incorporate these rules from now on! (I tried it in the correction commit to the indentation issue in my forked repo) Is there a way to correct the body of commits?
And do I have to create a new PR to introduce my changes to code or is there a way to introduce them in this PR?

ljvmiranda921 · 2018-06-23T13:58:22Z

Awesome!

It's fine @whzup !

I'm sorry I wasn't aware of such a style guide, I'll try to incorporate these rules from now on! (I tried it in the correction commit to the indentation issue in my forked repo) Is there a way to correct the body of commits?

Usually, if we want to edit commits, we use git rebase. You can use it to squash (fuse two or more commits together), reorganize, and reword your previous commits. If you want to modify the commit message, you should look for the reword command in git rebase.

Be careful though, and make sure that every time you commit, you are just editing one file (or subset of files). Rebase commands can cause conflicts within the history etc. But if you're just rewording your commit messages, then there's no problem.

Some helpful links:

In practice, what I usually do is to have [WIP] (Work In Progress) commits. So my commit messages before rebasing looks like this:

[WIP] Update files
[WIP] Add this method to file
[WIP] Add two more methods on same file
[WIP-test] Add test cases
[WIP-test] More tests

This categorizes commits related to the main file and the test files. So after rebasing, it will now look like this:

Add file for doing stuff
Add test cases for file that does stuff

But of course, your mileage may vary, check what works for you 👍

And do I have to create a new PR to introduce my changes to code or is there a way to introduce them in this PR?

Nope no need! Just commit your changes in whzup:pyramid_topology branch and it will automatically appear here!

@whzup

Resolves #136 Created an Inverse Kinematics Tutorial in a Jupyter Notebook. Committed with @whzup

ljvmiranda921

Hi @whzup!

Glad that it's working right now. 👍 I actually checked the Delaunay implementation before and I think it does resemble the Pyramid topology in literature. I just have some few comments below, and I think we will be all set.

ljvmiranda921 · 2018-06-26T11:54:00Z

pyswarms/backend/topology/pyramid.py

+                idx = np.array()
+                # Insert all the neighbors for each particle in the idx array
+                for i in range(swarm.n_particles):
+                    idx.append(indptr[indices[i]:indices[i+1]])


Awesome! Can you check if this can be golfed via list-comprehension?
As far as I know list comps are much faster than initializing an array and appending to it.

idx = [indptr[indices[i]:indices[i+1] for i in range(swarm.n_particles)]

Sure I guess that is possible! I'm currently struggling with the rebasing. I rebased a wrong commit and I'm trying to resolve the mess I did 😃. Could I just refork the repo and add the files again? Or is does this mess up anything?

Ok, nevermind got it working now! It's online now 👍.

Awesome! Yup just found it right now!

ljvmiranda921 · 2018-06-26T11:55:37Z

pyswarms/backend/topology/pyramid.py

+                best_cost = np.min(swarm.pbest_cost)
+            else:
+                pyramid = Delaunay(swarm.position)
+                indices, indptr = pyramid.vertex_neighbor_vertices


I wonder what indptrstands for? Index pointer? 😕

Maybe we can change this into a more understandable variable? 👍

Created a class for the implementation of a pyramid topology using Delaunay triangulation with a test file for the standard tests. Additionally, changed a small error in the description of the compute_velocity() method in the Ring class. See also: #129

@whzup

Resolves #136 Created an Inverse Kinematics Tutorial in a Jupyter Notebook. Committed with @whzup

…into pyramid_topology

Added a list comprehension for obtaining the indices and changed the variable name indptr to index_pointer. See also: #142

ljvmiranda921 · 2018-06-26T13:22:45Z

Okay, so what I will do now is squash all your commits into a single one. If you wish to contribute again, simply fork the development branch. Or perhaps pull the new changes in your own repository 👍

Wow, thanks for your contributions @whzup ! You're really nice to work with, I appreciate your help! The commits you've done will show up once we've merged everything to master and released v.0.3.0. I'm really thankful for all your help in improving PySwarms! Hope you enjoyed and learned something in the process!

whzup · 2018-06-26T15:26:45Z

Thanks! Really appreciate the working environment here, it's so friendly!

I have just been working on the tests for the pyramid class and found that there is a little bug and I'm not quite sure how to resolve it without using an ugly workaround. When the list with the indices of the neighbours is created it actually creates an array of floats:

idx = np.array([index_pointer[indices[i]:indices[i+1]] for i in range(swarm.n_particles)])

Additionally, the respective list with indices of the neighbours for every point does not always have the same length! So if I run the script I get an IndexError: arrays used as indices must be of integer (or boolean) type. So I tried to switch the data type of the array to integer but then it raises a ValueError: setting an array element with a sequence.

I thought about filling in the missing values. But then the question raises with what? If I just choose 0 it might yield a wrong result for the best neighbour and if I choose infinity it would raise an IndexError. Do you know an easy solution? I have searched for appropriate functions but I don't find anything useful. Shall I create another PR so you can inspect the code? I also added tests for the pyramid class and a topology attribute to the SwarmOptimizer class, such that you can use it as a constructor parameter.

ljvmiranda921 · 2018-06-26T15:51:14Z

Hmmm... let me check on this on the weekend. Yes please, can I see what tests you ran? Is this more of an edge-case or it’s a pretty regular thing given a certain input? For now let’s just isolate a PR for the Pyramid class first before the “topology attribute in the SwarmOptimizer.” Awesome! I might respond slowly in the next few days because of thesis. But I’ll get back to you this weekend.

On Wed, 27 Jun 2018 at 00:26 Aaron ***@***.***> wrote: Thanks! Really appreciate the working environment here, it's so friendly! I have just been working on the tests for the pyramid class and found that there is a little bug and I'm not quite sure how to resolve it without using an ugly workaround. When the list with the indices of the neighbours is created it actually creates an array of floats: idx = np.array([index_pointer[indices[i]:indices[i+1]] for i in range(swarm.n_particles)]) Additionally, the respective list with indices of the neighbours for every point does not always have the same length! So if I run the script I get an IndexError: arrays used as indices must be of integer (or boolean) type. So I tried to switch the data type of the array to integer but then it raises a ValueError: setting an array element with a sequence. I thought about filling in the missing values. But then the question raises with what? If I just choose 0 it might yield a wrong result for the best neighbour and if I choose infinity it would raise an IndexError. Do you know an easy solution? I have searched for appropriate functions but I don't find anything useful. Shall I create another PR so you can inspect the code? I also added tests for the pyramid class and a topology attribute to the SwarmOptimizer class, such that you can use it as a constructor parameter. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMWYs62dnb8HtOhw8b0725XShv4avKldks5uAlK3gaJpZM4U0tko> .

-- Lester James V. Miranda BS Electronics and Communications Engineering, Minor in Philosophy (Class of 2016) Ateneo de Manila University lester.miranda@ <[email protected]>*o <http://toki.waseda.jp>bf.ateneo.edu <http://bf.ateneo.edu>*

whzup · 2018-06-26T16:12:01Z

See #147. The tests are actually dependent on the topology attribute so I just left it in. Sorry for the commit mess in the new PR. I feel like Git is very unforgivable and I missed the opportunity to refork the new development branch.

ljvmiranda921 · 2018-06-26T16:26:45Z

It’s fine! I actually made a comment as well regarding this in #147. Haha! Let’s just make the discussion in that PR.

On Wed, 27 Jun 2018 at 01:12 Aaron ***@***.***> wrote: See #147 <#147>. The tests are actually dependent on the topology attribute so I just left it in. Sorry for the commit mess in the new PR. I feel like Git is very unforgivable and I missed the opportunity to refork the new development branch. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMWYs6NFLEsMUi2zfnWnzGMDMV50RSDlks5uAl1SgaJpZM4U0tko> .

-- Lester James V. Miranda BS Electronics and Communications Engineering, Minor in Philosophy (Class of 2016) Ateneo de Manila University lester.miranda@ <[email protected]>*o <http://toki.waseda.jp>bf.ateneo.edu <http://bf.ateneo.edu>*

ljvmiranda921 self-requested a review June 23, 2018 09:31

ljvmiranda921 self-assigned this Jun 23, 2018

ljvmiranda921 added bug Bugs, bug fixes, etc. enhancement Feature requests v0.3.0 unit tests Test-related changes labels Jun 23, 2018

ljvmiranda921 requested changes Jun 23, 2018

View reviewed changes

ljvmiranda921 assigned whzup Jun 23, 2018

ljvmiranda921 added documentation Documentation improvements or fixes and removed bug Bugs, bug fixes, etc. labels Jun 23, 2018

Fix indentation

f595feb

Fix indentation error in the Pyramid class that prevented the tests from running. See also: #142

whzup and others added 2 commits June 26, 2018 13:41

Create Inverse Kinematics tutorial (#141)

1585076

Resolves #136 Created an Inverse Kinematics Tutorial in a Jupyter Notebook. Committed with @whzup

Merge branch 'development' into pyramid_topology

ff2d63e

ljvmiranda921 reviewed Jun 26, 2018

View reviewed changes

whzup added 4 commits June 26, 2018 14:28

Create class for pyramid topology

8100888

Created a class for the implementation of a pyramid topology using Delaunay triangulation with a test file for the standard tests. Additionally, changed a small error in the description of the compute_velocity() method in the Ring class. See also: #129

Create Inverse Kinematics tutorial (#141)

9d766f6

Resolves #136 Created an Inverse Kinematics Tutorial in a Jupyter Notebook. Committed with @whzup

Merge branch 'pyramid_topology' of https://github.com/whzup/pyswarms …

4fc4938

…into pyramid_topology

Add list comprehension and better variable name

d69de25

Added a list comprehension for obtaining the indices and changed the variable name indptr to index_pointer. See also: #142

ljvmiranda921 approved these changes Jun 26, 2018

View reviewed changes

ljvmiranda921 merged commit 6bd896d into ljvmiranda921:development Jun 26, 2018

whzup deleted the pyramid_topology branch June 26, 2018 13:49

whzup mentioned this pull request Jun 26, 2018

Add choice of topology and tests for the pyramid topology #147

Closed

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

Created class for pyramid topology #142

Created class for pyramid topology #142

whzup commented Jun 23, 2018

ljvmiranda921 Jun 23, 2018

ljvmiranda921 Jun 23, 2018

ljvmiranda921 commented Jun 23, 2018 •

edited

Loading

whzup commented Jun 23, 2018

ljvmiranda921 commented Jun 23, 2018 •

edited

Loading

ljvmiranda921 left a comment

ljvmiranda921 Jun 26, 2018

whzup Jun 26, 2018

whzup Jun 26, 2018

ljvmiranda921 Jun 26, 2018

ljvmiranda921 Jun 26, 2018

ljvmiranda921 Jun 26, 2018

ljvmiranda921 commented Jun 26, 2018

whzup commented Jun 26, 2018

ljvmiranda921 commented Jun 26, 2018 via email

whzup commented Jun 26, 2018

ljvmiranda921 commented Jun 26, 2018 via email

Created class for pyramid topology #142

Created class for pyramid topology #142

Conversation

whzup commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljvmiranda921 commented Jun 23, 2018 • edited Loading

whzup commented Jun 23, 2018

ljvmiranda921 commented Jun 23, 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

ljvmiranda921 commented Jun 26, 2018

whzup commented Jun 26, 2018

ljvmiranda921 commented Jun 26, 2018 via email

whzup commented Jun 26, 2018

ljvmiranda921 commented Jun 26, 2018 via email

ljvmiranda921 commented Jun 23, 2018 •

edited

Loading

ljvmiranda921 commented Jun 23, 2018 •

edited

Loading