-
Notifications
You must be signed in to change notification settings - Fork 2
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
Testing improvements and a clarification to the interface #13
Conversation
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.
Looks good, I support clarifying the addition over zeroing of forces since I had a case where addition was explicitly required.
test_energy_forces(sys, calculator; force_eltype=nothing, rtol=1e8, kwargs...) | ||
|
||
Test your calculator for AtomsCalculators interface. Passing test means that your | ||
forces calculation. |
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.
Incomplete sentence (and below)?
This PR will solve couple of issues with testing and add combined testing functions that allow testing everything with a one call.
forces!
takes in an array where forces are saved. It can be overwritten or added. It was not defined which one it should be. Now addition is defined and also tested.energy_forces
andenergy_forces_virial
where not tested with test functions. This has now been updated with new functionstest_energy_forces
and testenergy_forces_virial
. The new function include the corresponding individual tests in them, so you should use them over the individual calls.With this the original version (v0.1) of AtomsCalculators should be complete and we can start to implement new functionality (#12) to AtomsCalculators.
Couple of notes:
Could we get couple of fast approvals to get this one done fast, so that we can move forward to new funcitonality @cortner, @mfherbst, @rkurchin @jgreener64
edit. updated 3. point on the descriptions