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

Update Amber test script/reference test output files #353

Closed
Madu86 opened this issue Mar 22, 2024 · 2 comments · Fixed by #355
Closed

Update Amber test script/reference test output files #353

Madu86 opened this issue Mar 22, 2024 · 2 comments · Fixed by #355
Assignees
Labels
bug Something isn't working

Comments

@Madu86
Copy link
Collaborator

Madu86 commented Mar 22, 2024

The tests on Jenkins failed due to an update we made to QUICK output. To fix this, we must either update reference test output files (https://github.com/merzlab/QUICK/tree/master/test/saved) or the Runtest amber script (https://github.com/merzlab/QUICK/blob/master/test/Run.tests.amber). Can you take care of this quickly? To reproduce the test failure, simply compile the code, source quick.rc from the installation directory, go to the tests directory and run Run.tests.amber script.

Thanks!

@Madu86 Madu86 added the bug Something isn't working label Mar 22, 2024
@Madu86
Copy link
Collaborator Author

Madu86 commented Mar 22, 2024

I updated reference files with pipe symbols but optimization tests and F tests are still failing. This is because 1) someone removed max geom opt cycles and generated reference .out files., 2) a trap for F tests is not implemented in the above Runtest script. I implemented one in /tools/runtest script. The same has to be done here. @akhilshajan, @ohearnk Can you take care of these tasks and update 353-update-amber-test-scriptreference-test-output-files branch?

@agoetz
Copy link
Collaborator

agoetz commented Mar 23, 2024

We should get rid of the Run.tests.amber script. It duplicates functionality of the runtest script. We should use the runtest script instead. When I use the runtest script with appropriate modification in the Makefile that runs AmberTools tests, all test pass in Amber Gitlab branchquick-amber (only tested for serial and MPI builds).

This works because we do not do a diff on the entire file, but just on .ene, .grad, and .opt files that contain the relevant data. Furthermore, the runtest script has a trap for F tests.

Only a minor modification to the runtest script is required. I created PR #354 for this.

Two things need to be done in Amber Gitlab branch quick-amber, @Madu86 can you do that after incorporating the updated runtest script?

  1. Modify the Amber CMake to copy the runtest script into $AMBERHOME/AmberTools/src/quick and do not copy Run.tests.amber (we should just remove it).
  2. Modify $AMBERHOME/AmberTools/test/Makefile to call runtest with appropriate flags (e.g. --serial) instead of Run.tests.amber

The corresponding section in $AMBERHOME/AmberTools/test/Makefile should look as follows:

test.Quick:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --serial ;\
        fi ;\
        )

test.Quick.MPI:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --mpi ;\
        fi ;\
        )

test.Quick.cuda:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --cuda ;\
        fi ;\
        )

test.Quick.cuda.MPI:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --cudampi ;\
        fi ;\
        )

test.Quick.hip:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --hip ;\
        fi ;\
        )

test.Quick.hip.MPI:
        @(if [ "$(QUICK)" = 'yes' ]; then \
        cd ../src/quick && ./runtest --hipmpi ;\
        fi ;\
        )

@Madu86 Madu86 linked a pull request Mar 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants