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

Code in mathematician for check_convergence may not be right #70

Open
kwalcock opened this issue Nov 3, 2023 · 2 comments
Open

Code in mathematician for check_convergence may not be right #70

kwalcock opened this issue Nov 3, 2023 · 2 comments

Comments

@kwalcock
Copy link
Collaborator

kwalcock commented Nov 3, 2023

There may be an absolute value or two missing in

def check_convergence(centroids, last_centroids, j):
diffs = []
for i, centroid in enumerate(centroids):
diffs.append(np.mean(centroid - last_centroids[i]))
if max(diffs) < 0.00001 or j > 100:
return True
else:
return False

If a centroid and last_centroid are [0, 0, 0] and [10, 10, 10], then centroid - last_centroid is [-10, -10, -10] with a mean of -10, which is certainly less than 0.00001.

If a centroid and last_centroid are [0, 0, 0] and [10, 0, -10], then centroid - last_centroid is [-10, 0, 10] with a mean of 0, which is also less than 0.00001.

It doesn't seem like that fits the definition of convergence.

The code may have to be

diffs.append(np.abs(np.mean(centroid - last_centroids[i])))

for the first issue or even

diffs.append(np.mean(np.abs(centroid - last_centroids[i])))

to take care of them both.

@kwalcock
Copy link
Collaborator Author

kwalcock commented Nov 5, 2023

@Allegra-Cohen, in case you are only notified when directly addressed.

@Allegra-Cohen
Copy link
Owner

@kwalcock you're right! I think I threw this function together quite quickly so pretty sure I didn't have a reason to skip abs other than making a mistake. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants