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.

Sign up for GitHub

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

Jump to bottom

Refactor codebase #246

Merged
merged 9 commits into from
Oct 20, 2018
Merged

Refactor codebase #246

merged 9 commits into from
Oct 20, 2018

Conversation

ljvmiranda921
Copy link
Owner

@ljvmiranda921 ljvmiranda921 commented Sep 12, 2018 •
edited
Loading

Description

A large pull request that deals mainly with refactoring some parts of the codebase. Specifically:

  • The tests for topology and high-level optimizers
  • Error-handling from LBYL to EAFP
  • Remove if-else hierarchy in General Optimizer

Related Issues

#232, #240, #241

Motivation and Context

It is important that we make our code scalable, and has a consistent design before we add bigger features.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ljvmiranda921 ljvmiranda921 changed the title General Refactor PR General Refactor Sep 12, 2018
@ljvmiranda921 ljvmiranda921 changed the base branch from master to development September 12, 2018 08:05
@ljvmiranda921
Copy link
Owner Author

Fix GeneralOptimizer -> np.min problem

@ljvmiranda921 ljvmiranda921 changed the title General Refactor [SPRINT] Refactor codebase Sep 12, 2018
@ljvmiranda921 ljvmiranda921 changed the title [SPRINT] Refactor codebase [WIP] [SPRINT] Refactor codebase Sep 12, 2018
@ljvmiranda921
Refactor GeneralOptimizer if-else hierarchy (#232)
277d9c0 
This commit removes the if-else hierarchy in GeneralOptimizer
by abstracting some parts of the topologies.
Resolves #232

Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Refactor test abstractions (#240) (#241)
ad80287 
This commit adds test abstractions to PySwarms
Resolves #240, #241

Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Add pytest-cov config file
0b36637 
This commit adds a .coveragerc file to set-up the coverage
requirements when running pytest.
Related #245

Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Add isort configuration
569c8a7 
This commit adds an isort configuration to ensure that isort works out
of the box.

Signed-off-by: Lester James V. Miranda <[email protected]>
whzup
whzup reviewed Oct 1, 2018
View reviewed changes
.coveragerc Outdated
<<<<<<< HEAD
pos = init_pos
=======
>>>>>>> 092ad40... Add pytest-cov config file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is still a merge conflict 😄

Copy link
Owner Author

Choose a reason for hiding this comment

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

Whoops, can you check if there are still instances of this in other files?

whzup
whzup reviewed Oct 1, 2018
View reviewed changes
pyswarms/utils/decorators/decorators.py Outdated
)
assert vector.shape == (
n_particles,
), "The cost function should return a single value."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should remove this assert statement 🤔 We could replace it with a try-except.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes you’re correct, but this is more of a validator that ensures vector.shape == (n_particles,), maybe we can do a LBYL style here? ( Let’s just remove asserts since assert statements can be skipped during runtime) 👍

@ljvmiranda921
Fix merge conflict in .coveragerc
a359feb
@ljvmiranda921
Add init_pos in Optimizer docstrings
e5d7d93 
This commit adds the `init_pos` argument in the Optimizer
docstrings. Turns out that it's not documented in the high-level
optimizers, so here we go.

Related #251

Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Update test on decorators
32cb893 
This commit removes the assert statement when verifying if
the resulting cost actually has the desired shape. Moreover,
additional tests were created to test this validator.

Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Copy link
Owner Author

Just check this one first, @whzup , then we can push this to development. I'm planning to work on adding random seeds first (would you like to work on that, or maybe @SioKCronin ?) then merge the BoundaryHandlers, then the topology fixes. What do you think?

whzup
whzup reviewed Oct 3, 2018
View reviewed changes
Copy link
Collaborator

@whzup whzup left a comment

Choose a reason for hiding this comment

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

I looked through it and it seems alright to me 👍 Just wondering if the Import from ... comments are necessary 🤔

pyswarms/backend/topology/star.py Outdated
# Update best_cost and position
swarm.best_pos, swarm.best_cost = my_topology.compute_best_particle(my_swarm)
# Update best_cost and position
swarm.best_pos, swarm.best_cost = my_topology.compute_best_particle(my_swarm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does compute_best_particle exist?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Whoops. No it does not exist. Let me fix it

@whzup
Copy link
Collaborator

whzup commented Oct 3, 2018

would you like to work on that

I'd like to continue with the Handlers first 😄 I think there is still quite some work to do especially with the tests etc.

@ljvmiranda921
Fix docstring in Star topology
c289bdd 
Replaces compute_best_particle with compute_gbest.
Signed-off-by: Lester James V. Miranda <[email protected]>
@ljvmiranda921
Copy link
Owner Author

would you like to work on that

I'd like to continue with the Handlers first smile I think there is still quite some work to do especially with the tests etc.

Sure thing! I will check if I can handle the random seed 👍

whzup
whzup approved these changes Oct 19, 2018
View reviewed changes
Copy link
Collaborator

@whzup whzup left a comment

Choose a reason for hiding this comment

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

This one looks good! 👍 I reckon it's ready for merging 😄

@ljvmiranda921
Copy link
Owner Author

Thanks a lot @whzup ! Sorry if I got a bit busy. Just started working this monday! haha

@ljvmiranda921 ljvmiranda921 changed the title [WIP] [SPRINT] Refactor codebase Refactor codebase Oct 20, 2018
@ljvmiranda921 ljvmiranda921 merged commit 7e4d04c into development Oct 20, 2018
@ljvmiranda921 ljvmiranda921 deleted the refactor/general branch January 29, 2019 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whzup whzup whzup approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ljvmiranda921 @whzup