Allegra-Cohen / grid

GNU General Public License v3.0
1 stars 3 forks source link

Code in mathematician for check_convergence may not be right #70

Open kwalcock opened 1 year ago

kwalcock commented 1 year ago

There may be an absolute value or two missing in https://github.com/Allegra-Cohen/grid/blob/8670a1ed8255a770d351efb5885f445d816ddc78/habitus_ui_interface-main/backend/mathematician.py#L287-L294

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 commented 1 year ago

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

Allegra-Cohen commented 1 year ago

@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!