Regularization in higher dimensions#354
Conversation
|
Thank you @YuanbinLiu ! Some tests are still failing. Can we handle anything beyond 4D? If not, should we add this limitation to the documentation? |
It's not limited to 4D; higher dimensions are also fesible. I'll find time to check for errors, but the tests passed before I pushed the PR. |
|
@YuanbinLiu Thanks for the explanation! Test results can be dependent on the operating system |
I have tried several times, all passed. I didn't found some bugs. Can anyone test it on a different system? |
| f"Point and points must have the same dimensionality. Got {pn.shape[0]} and {preg.shape[1]}." | ||
| ) | ||
| hull = ConvexHull(preg) | ||
| return np.all(np.dot(hull.equations[:, :-1], pn) + hull.equations[:, -1] <= 1e-12) |
There was a problem hiding this comment.
Is this floating point comparison maybe brittle?
There was a problem hiding this comment.
It shouldn't be caused by this. I tested it on different computers with new environments, and it worked fine.
| fraction_list = [[1.0]] + [[0.0]] + [[0.5]] * 8 | ||
| calc_hull_ND = calculate_hull_nd(label) | ||
|
|
||
| assert np.allclose(calc_hull_3D.equations, calc_hull_ND.equations, atol=1e-6) |
| get_e_dist_hull_ND = get_e_distance_to_hull_nd( | ||
| calc_hull_ND, atom, {3: -0.28649227, 17: -0.25638457}, "REF_energy" | ||
| ) | ||
| assert np.allclose(get_e_dist_hull_3D, get_e_dist_hull_ND, atol=1e-6) |
|
@YuanbinLiu could you check all numerical tolerances again and especially for the failing test? It is likely just the tolerance |
| ] | ||
|
|
||
| assert all(d >= -1e-6 for d in des) | ||
| assert np.all(np.array(des) >= -1e6) |
There was a problem hiding this comment.
Oh yes, missed that -, thats why test passed seems, I just saw the both methods returns 1e6 if it fails
https://github.com/YuanbinLiu/autoplex_pub/blob/76c29c6c3787c1cc4cd30ec13c3f42969da6296a/src/autoplex/fitting/common/regularization.py#L653
https://github.com/YuanbinLiu/autoplex_pub/blob/76c29c6c3787c1cc4cd30ec13c3f42969da6296a/src/autoplex/fitting/common/regularization.py#L653
There was a problem hiding this comment.
If some configurations have a distance from the convex hull exceeding 1e6, they will be excluded from the training set afterwards.
Are the values supposed to be non zero, positive , negative ? Is there any specific range this values are suppose to be ? Also with this new function added supporting n dimensions, do we still need 3d method or it can be replaced with this new function now ? Can you comment on this @YuanbinLiu ? |
They should be non-negative (i.e., >= 0). Actually, we no longer need 3D, as this has already been handled in the new function. |
| regularization: bool = False, | ||
| retain_existing_sigma: bool = False, | ||
| scheme: str = "linear-hull", | ||
| element_order: list = None, |
There was a problem hiding this comment.
| element_order: list = None, | |
| element_order: list | None = None, |
list = None would cause a data type mismatch for list
| norm = np.cross(n_d[2] - n_d[0], n_d[1] - n_d[0]) | ||
| plane_norm = norm / np.linalg.norm(norm) | ||
| else: | ||
| A = n_d[:-1] - n_d[0] |
There was a problem hiding this comment.
maybe clear variable names would be a bit better?
There was a problem hiding this comment.
Thanks. Changed.
|
Hi @YuanbinLiu , seems fine to me now, just wondering if we still need this method now ? https://github.com/YuanbinLiu/autoplex_pub/blob/a8bddf095e72ba300f7046ab347218feae6a5f20/src/autoplex/fitting/common/regularization.py#L529 As the new one added should basically work or any case. Maybe we can remove it, as we will not use it? Also tagging @JaGeo , if you have any thoughts on it as well. |
|
If the new method replaces it completely, we should probably remove it as we will otherwise lose track of it |
| element_order: list | None | ||
| List of atomic numbers in order of choice (e.g. [42, 16] for MoS2) |
There was a problem hiding this comment.
could we determine this automatically?
There was a problem hiding this comment.
Good question. I would say it's definitely doable, but modifying it involves more code changes. I'll be updating some features of RSS in the next PR, and I'll include this functionality in that update.
| - (plane_constant - np.dot(plane_norm[:-1], sp[:-1])) / plane_norm[-1] | ||
| ) | ||
|
|
||
| print("Failed to find distance to hull in ND") |
There was a problem hiding this comment.
Ahh, yes, done.
| scheme: str | ||
| Scheme to use for regularization. | ||
| element_order: | ||
| List of atomic numbers in order of choice (e.g. [42, 16] for MoS2) |
There was a problem hiding this comment.
could we extend the doc string? I am not sure I fully understand what this is for (e.g., the sigma)
There was a problem hiding this comment.
This is introduced to support solving high-dimensional energy convex hulls. Sure, I will extend it accordingly.
| energy = ( | ||
| atoms.info[energy_name] / len(atoms) | ||
| if energy_name != "energy" | ||
| else atoms.get_potential_energy() / len(atoms) |
There was a problem hiding this comment.
Why do we need this additional else condition? Wouldn't this fail if no calculator is attached to the atom's object? or it still works fine?
There was a problem hiding this comment.
This is because in the new version of ASE, if the energy label is "energy", it no longer supports reading from info, but instead uses get_potential_energy(). However, if the user specifies a different energy label, it will be read from info according to their setting.
There was a problem hiding this comment.
Oh okay. Thanks did not know this was changed.
Good suggestions. They were safely removed now. |
|
Thanks! |
4 participants
We have upgraded the functionality to support high-dimensional convex hull calculations with regularization. Previously, it was limited to 3D convex hulls, but we are now able to handle higher-dimensional cases, such as 4D convex hulls for three-element systems.
Test 1: Verify whether the new code can produce the same convex hull results as the old version for the 3D case.
Test 2: Evaluate whether the new code can handle high-dimensional (>3D) convex hulls.