Skip to content
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

Feature parameterize obj #144

Merged

Conversation

bradahoward
Copy link
Contributor

#143

Implementation
Used both *args and **kwargs so user can choose which one they prefer. Added to GlobalBestPSO, LocalBestPSO and BinaryPSO. Also added an example IPython notebook to demonstrate usage.

@ljvmiranda921
Copy link
Owner

Hi @bradahoward !

Awesome PR, I appreciate the unit tests and the updates in documentation. Some comments:

  1. I think the best way to include *args and **kwargs is to include them directly in the optimize() function rather than having a positional argument with name args=() or kwargs={}:
def optimize(f, iters, print_step, verbose, *args, **kwargs):
    # Stuff...
    self.swarm.current_cost = objective_func(self.swarm.position, *args, **kwargs)
    self.swarm.pbest_cost = objective_func(self.swarm.pbest_pos, *args, **kwargs)

Would you mind testing this one out? Using *args and **kwargs is more Pythonic and minimizes code smells 👍 . Can you do the same thing with the search module? If not, just revert that module to the original. I'm planning to update its API in the future.

So as far as I know, we just need to update the base module and the single module by adding the *args and **kwargs in the optimize method.

  1. The environment module will be deprecated. No need to update or touch that. You can just revert the commit on that file so we won't have any merge conflicts down the road. 👍

@@ -193,7 +193,7 @@ def _populate_history(self, hist):
self.pos_history.append(hist.position)
self.velocity_history.append(hist.velocity)

def optimize(self, objective_func, iters, print_step=1, verbose=1):
def optimize(self, objective_func, iters, args=(), kwargs={}, print_step=1, verbose=1):
"""Optimizes the swarm for a number of iterations.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe *args and **kwargs will work here instead of args=() and kwargs={}?

@@ -224,7 +224,7 @@ def _populate_history(self, hist):
self.pos_history.append(hist.position)
self.velocity_history.append(hist.velocity)

def optimize(self, objective_func, iters, print_step=1, verbose=1):
def optimize(self, objective_func, iters, args=(), kwargs={}, print_step=1, verbose=1):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe *args and **kwargs will work here instead of args=() and kwargs={}?

@@ -149,7 +149,7 @@ def __init__(
# Initialize the topology
self.top = Ring()

def optimize(self, objective_func, iters, print_step=1, verbose=1):
def optimize(self, objective_func, iters, args=(), kwargs={}, print_step=1, verbose=1):
"""Optimizes the swarm for a number of iterations.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe *args and **kwargs will work here instead of args=() and kwargs={}?


cli_print("Arguments Passed to Objective Function: \nargs: %s \nkwargs: %s\n" % (args, kwargs),
verbose, 2, logger=self.logger)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, keep this. I'd suggest using:

cli_print("Arguments passed to objective function: \nargs: %s \nkwargs: %s\n" % (*args, **kwargs),
                  verbose, 2, logger=self.logger)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that using ** outside of a function call throws an IndexError. The Python documentation around this uses them without the decorators in the outside of the function call.
https://docs.python.org/dev/tutorial/controlflow.html#more-on-defining-functions
Thoughts?

Copy link
Owner

@ljvmiranda921 ljvmiranda921 Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. Yes, you are correct, just remove ** and it should print. One last Python 3.X thing, perhaps a .format() is better?

name = 'ljvmiranda921'
print("Hi my name is: {}".format(name))

I'd probably update my cli_print soon to make everything uniform.

self.swarm.current_cost = objective_func(self.swarm.position)
self.swarm.pbest_cost = objective_func(self.swarm.pbest_pos)
self.swarm.current_cost = objective_func(self.swarm.position, *args, **kwargs)
self.swarm.pbest_cost = objective_func(self.swarm.pbest_pos, *args, **kwargs)
self.swarm.pbest_pos, self.swarm.pbest_cost = compute_pbest(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this is just the modification we need. That is, to provide flexibility in the objective_function by providing an optional *args or **kwargs argument. There's no need to put args = () or kwargs = {} in the main optimize() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I fixed the optimize prototypes. The only goofy thing there is the *args had to be listed before the other key word arguments i.e. def fun(foo, bar, *args, baz=3, **kwargs)

@@ -104,7 +104,7 @@ def assertions(self):
"Missing methods in optimizer, check " "pyswarms.base module"
)

def __init__(self, optimizer, objective_func, iters):
def __init__(self, optimizer, objective_func, iters, obj_args=(), obj_kwargs={}):
"""Runs the optimizer against an objective function for a number
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to touch this 👍 This will be Deprecated soon. Simply revert this to the original to avoid merge conflicts down the road.

@@ -33,6 +33,8 @@ def __init__(
options,
objective_func,
iters,
obj_args=(),
obj_kwargs={},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to avoid obj_args=() and obj_kwargs={}? If not, just revert this to the original. I'm planning to update its API soon

from pyswarms.single import GlobalBestPSO, LocalBestPSO
from pyswarms.utils.functions.single_obj import rosenbrock_func


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome tests! Just a few more tests to write:

  1. Can you please test if an error is raised when *args/**kwargs is passed but it's not defined in the objective function itself? (e.g. I passed c and d but rosenbrock accepts a and b).
  2. Can you also test if error is raised when not all *args/**kwargs are supplied? (e.g. I only passed a but rosenbrock accepts both a and b)
  3. Test what happens if I passed more than one *args/**kwargs than necessary.

@ljvmiranda921
Copy link
Owner

Thanks for this @bradahoward! I'll go over this PR hopefully this weekend, just need to finish this week, etc. Noted on your questions, I will try some variants on my end and let's see what works.

@ljvmiranda921
Copy link
Owner

ljvmiranda921 commented Jun 27, 2018

Hi @bradahoward ,

I'm thinking of just using **kwags instead. Adding positional arguments may introduce nasty errors, and whenever we parametrize our objective function, we often add named arguments, right? Any thoughts?

# Sample usage
def f(x, a, b):
    """An objective function with two params a and b"""
    j = (x**2.0).sum(axis=1)
    return (a * j + b)

optimizer = GlobalBestPSO(n_particles=10, dimensions=2, options)
optimizer.optimize(f, iters=100, a=5, b=6)

No need for *args, might make things confusing. Let's just have the user explicitly define the parameter arguments of his/her custom-objective function as keyword args.

This is a short demo on how I imagine it to be used: https://repl.it/@ljvmiranda921/PySwarmsKwargs

  • Remove *args, it might introduce unexpected behavior
  • Format strings via .format(). This is the new style formatting.
  • Add more tests based on previous comments.

@bradahoward
Copy link
Contributor Author

hi @ljvmiranda921 -- thanks for the quick feedback!

If it were me, i would only use **kwargs. I like the explicit definition of the parameters. I will change this up today.

For the .format() on the logging i followed what you had in the print_step cli_print call . I can change that as well.

change to only **kwargs implementation, changed to .format() string method, and fixed tests
@ljvmiranda921
Copy link
Owner

ljvmiranda921 commented Jun 28, 2018

Hi @bradahoward ,

Awesome! Everything looks good to me (LGTM) now. You're correct, I think explicitly naming the arguments via **kwargs is much better. If you wish to change the string formats in the print functions, it's fine, just open up another PR if you want.

I will squash all your commits into a single one. Just update your own branch after I have merged your work into development

Anyway, super thankful for your help and contribution, I appreciate your help and you're very easy to work with!

@ljvmiranda921 ljvmiranda921 merged commit 89f12cb into ljvmiranda921:development Jun 28, 2018
@ljvmiranda921 ljvmiranda921 mentioned this pull request Jul 31, 2018
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants