-
Notifications
You must be signed in to change notification settings - Fork 2
change to unwrap_inputs round to 16 decimal places was added #23
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
Changes from all commits
5375d07
2c6c0d5
d0c1086
e83a2c0
fa32076
1426ad8
bf4f410
ad54823
0bc9522
a8c65f6
50d5d5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,11 +13,43 @@ | |
| # limitations under the License. | ||
|
|
||
| from simde import TotalEnergy, EnergyNuclearGradientStdVectorD | ||
| import numpy as np | ||
|
|
||
|
|
||
| def _compare_mol_and_point(mol, points): | ||
| """This function is essentially a work around for the comparisons not being | ||
| exposed to Python. | ||
| def _compare_mol_and_point(mol, points, atol=1e-12, rtol=0.0): | ||
| """ | ||
| Compare the 3D nuclear coordinates of a Molecule and a PointSet. | ||
|
|
||
| This function is intended to ensure that gradient calculations | ||
| are being performed at the correct molecular geometry. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| mol : chemist.Molecule | ||
| The molecule whose nuclear coordinates are being compared. | ||
|
|
||
| points : chemist.PointSet | ||
| The point set to compare against (usually created from the molecule's nuclei). | ||
|
|
||
| atol : float, optional | ||
| Absolute tolerance used by np.isclose. Default is 1e-12. | ||
| This catches numerical noise due to Python/C++ floating-point boundary. | ||
|
|
||
| rtol : float, optional | ||
| Relative tolerance used by np.isclose. Default is 0.0 (disabled), | ||
| which ensures no scaling with magnitude — appropriate for comparing coordinates. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True if all coordinate components match within the given tolerances. | ||
| False otherwise. | ||
|
|
||
| Notes | ||
| ----- | ||
| Exact floating-point equality is not used because of small differences | ||
| (e.g., ~1e-314) introduced by Python/C++ interoperability. These are | ||
| not chemically meaningful and should be tolerated with a small `atol`. | ||
| """ | ||
| if mol.size() != points.size(): | ||
| return False | ||
|
|
@@ -27,13 +59,22 @@ def _compare_mol_and_point(mol, points): | |
| point_i = points.at(i) | ||
|
|
||
| for j in range(3): | ||
| if atom_i.coord(j) != point_i.coord(j): | ||
| return False | ||
| a = atom_i.coord(j) | ||
| b = point_i.coord(j) | ||
|
|
||
| if rtol == 0.0 and atol == 0.0: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check can run into problems with floating-point representation of hard-zero. Please use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as before, not sure if I need to leave it the way it was |
||
| # Strict comparison: values must be bitwise identical | ||
| if a != b: | ||
| return False | ||
| else: | ||
| # Use np.isclose to allow for floating-point tolerance | ||
| if not np.isclose(a, b, atol=atol, rtol=rtol): | ||
| return False | ||
|
|
||
| return True | ||
|
|
||
|
|
||
| def unwrap_inputs(pt, inputs): | ||
| def unwrap_inputs(pt, inputs, atol=1e-12, rtol=0.0): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You shouldn't be changing this method. The call to If this is where your error was originating from (as opposed to you trying to reuse
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as before not sure if I need to leave it the way it was |
||
| """ Code factorization for unwrapping a module's inputs. | ||
|
|
||
| Many of our friends expose interfaces which are analogous to high-level | ||
|
|
||
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.
This is NOT defaulting to the original behavior. The original behavior was exact equality.
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.
Not sure then what was the first comment about having a conditional, the first time I shared the error, that Is why I change the function and add a conditional for equality if needed.
Don't know what to do from here