Closed drkupi closed 5 years ago
Merging #32 into master will increase coverage by
0.22%
. The diff coverage is93.72%
.
@@ Coverage Diff @@
## master #32 +/- ##
==========================================
+ Coverage 92.3% 92.53% +0.22%
==========================================
Files 8 8
Lines 1365 1634 +269
==========================================
+ Hits 1260 1512 +252
- Misses 105 122 +17
Impacted Files | Coverage Δ | |
---|---|---|
pySOT/utils.py | 92.89% <100%> (+3.9%) |
:arrow_up: |
pySOT/auxiliary_problems.py | 91.78% <100%> (+0.05%) |
:arrow_up: |
pySOT/strategy.py | 89.73% <91.86%> (+1.32%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update aab0f98...c75f842. Read the comment docs.
Hi David,
I have attempted to resolve all of your specific and general comments. Here is the update on the general comments:
Code styling has been improved and checked using flake8
I have tried to remove redundant code snippets.
The overall code coverage has been improved to 93%.
Hi Taimoor,
The code mostly looks good! I've a few minor comments that shouldn't take long to fix.
@dbindel: Do you think you'll have a chance to review the PR in the next few days so we can merge it?
I made changes as per your feedback, and updated the pull request.
All changes are done.
On Thu, Apr 25, 2019 at 5:17 AM David Eriksson notifications@github.com wrote:
@dme65 requested changes on this pull request.
Comments
In pySOT/utils.py https://github.com/dme65/pySOT/pull/32#discussion_r278321260:
- """
- flag = 1
- for i in range(nc):
- if X_c[i, dim+4] == 0:
- sigma_new = sigma
- else:
- sigma_new = np.power(1/2, X_c[i, dim+4])*sigma
- d = scp.distance.euclidean(X, X_c[i, 0:dim])
- if d < sigma_new*d_thresh/np.sqrt(len(X)):
- flag = 0
- break
- return flag
+class SopRecord():
Can you move this back to strategy.py and rename it _SopRecord? Same for SopCenter
In pySOT/utils.py https://github.com/dme65/pySOT/pull/32#discussion_r278321297:
- while j < ndf_count:
- if domination(F[:, N], F[:, ndf_index[j-1]], M):
- df_index.append(ndf_index[j-1])
- ndf_index.remove(ndf_index[j-1])
- ndf_count -= 1
- elif domination(F[:, ndf_index[j-1]], F[:, N], M):
- df_index.append(N)
- ndf_index.remove(N)
- ndf_count -= 1
- break
- else:
- j += 1
- return (ndf_index, df_index)
+def ND_Front(F):
Rename to nd_front
In pySOT/utils.py https://github.com/dme65/pySOT/pull/32#discussion_r278321370:
- :type M: int
- :return: Boolean check for domination
- :rtype: Bool
- """
- d = False
- for i in range(0, M):
- if fA[i] > fB[i]:
- d = False
- break
- elif fA[i] < fB[i]:
- d = True
- return d
+def ND_Add(F, df_index, ndf_index):
Rename to nd_add
In pySOT/utils.py https://github.com/dme65/pySOT/pull/32#discussion_r278321618:
@@ -390,3 +390,182 @@ def optimize(self): population = new_population
return best_individual, best_value
+ +POSITIVE_INFINITY = float("inf") + +def domination(fA, fB, M):
- """Returns True if a fA dominates fB
- :param fA: Vector A of objective functions
- :param fB: Vector B of objective functions
- :param M: Number of objective functions in A and B
- :return: Boolean
- """
I prefer the more compact and Pythonic version, which will likely be faster as well due to interpreter overhead.
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/dme65/pySOT/pull/32#pullrequestreview-230360750, or mute the thread https://github.com/notifications/unsubscribe-auth/AEB7SHFI36SMTJBEXJWAK43PSDE5ZANCNFSM4HFFC4TQ .
Thanks, Taimoor!
LGTM
Added the SOP Strategy to the list of strategies in pySOT. Key updates include a new class SOPStrategy, and some supporting functions in the utils.py file.