-
Notifications
You must be signed in to change notification settings - Fork 333
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
Fix duplicate cost function calls in optimize() implementations #266
Fix duplicate cost function calls in optimize() implementations #266
Conversation
Hey @whzup , wanna help me out reviewing this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good solution. Seems like we had some redundancies there. Although, @ljvmiranda921 do you want this to go straight into the master
branch?
Hi @whzup ! Yes, I think we can treat this as a bugfix and release immediately. OK, I'll leave this to your judgement. If you're fine with this then I'll make the merge to master |
@ljvmiranda921 I think this works as it is, the tests run without errors so let us merge it 😄 |
@whzup ok then! will merge this now and do a patch release |
@all-contributors please add @danielcorreia96 for bug and code |
I've put up a pull request to add @danielcorreia96! 🎉 |
@all-contributors please add @whzup for code, infrastructure, doc, and ideas |
I've put up a pull request to add @whzup! 🎉 |
Description
Fixed duplicate cost function calls in optimize() implementations using a swarm pbest_cost initialization to np.inf.
Related Issue
Resolves: #257
Motivation and Context
As pointed out in #257, for each iteration of the algorithm, the cost function is being called twice in the current implementations. This leads to significant performance drawbacks in situations where the cost function is expensive to calculate.
Hence, we can just initialize the pbest_cost to np.inf in order to remove the second cost function call.
Full credits to @pdiaz2 for raising the issue and proposing this solution: I simply extracted the relevant changes from the solution provided via file (#257 (comment))
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Questions