-
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
Conversation
| 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): |
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
| a = atom_i.coord(j) | ||
| b = point_i.coord(j) | ||
|
|
||
| if rtol == 0.0 and atol == 0.0: |
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 check can run into problems with floating-point representation of hard-zero. Please use None as the default values for atol and rtol and switch this line to check for None and not 0.0.
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.
Same comment as before, not sure if I need to leave it the way it was
|
|
||
|
|
||
| def unwrap_inputs(pt, inputs): | ||
| def unwrap_inputs(pt, inputs, atol=1e-12, rtol=0.0): |
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.
You shouldn't be changing this method. The call to _compare_mol_and_point should always use exact equality. If the user provides even slightly different molecule objects, they are NOT requesting the derivative at the same point.
If this is where your error was originating from (as opposed to you trying to reuse _compare_mol_and_point, which is what I thought the problem was), then I suspect that the problem is that you're not passing the PointSet object from your Molecule object into your module.
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.
Same as before not sure if I need to leave it the way it was
|
The problem that motivated this PR has been resolved with NWChemEx/Chemist#450. I'm gonna close this now, but it can be reopened and completed if this functionality is still desired. |
Is this pull request associated with an issue(s)?
Yes, based on an error representation I think between python and c++ of the number zero.
Please list issues associated with this pull request, including closing words
as appropriate.
In the communication between python and c++ it looks like the number zero changes from 0.0 to 4.423185485e-315. This was observed for the coordinates of the (0.0, 0.0, 0.0) vector. So instead of the zero vector it was show the vector:
([4.423185485e-315, 4.42424859e-315, 4.41864936e-315 )
Description
Keeps the zero vector as it should by using round to 16 decimal places in:
'
def _compare_mol_and_point(mol, points):
"""This function is essentially a work around for the comparisons not being
exposed to Python.
"""
if mol.size() != points.size():
return False
'
TODOs
I think there is nothing else to do.