Skip to content
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

Merged
merged 11 commits into from
May 10, 2024

Conversation

tjjarvinen
Copy link
Collaborator

@tjjarvinen tjjarvinen commented May 7, 2024

This PR will solve couple of issues with testing and add combined testing functions that allow testing everything with a one call.

  1. non-allocating force 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.
  2. Combination calls energy_forces and energy_forces_virial where not tested with test functions. This has now been updated with new functions test_energy_forces and test energy_forces_virial. The new function include the corresponding individual tests in them, so you should use them over the individual calls.
  3. Added relative tolerance keyword for testing functions. This is needed, if there are different implementations to e.g. allocating and non-allocating forces. For consistency the keyword is implemented on all the 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:

  • We need to write more documentation. But this can be postponed untill we have added the new functionality
  • The plan is to tag this to v0.1.2 and then have the new features in for v0.2

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

Copy link

@jgreener64 jgreener64 left a 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete sentence (and below)?

@cortner cortner self-requested a review May 9, 2024 18:20
@tjjarvinen tjjarvinen merged commit 884a488 into JuliaMolSim:master May 10, 2024
4 checks passed
@tjjarvinen tjjarvinen deleted the testing_fixes branch May 10, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants