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

Implemented "count in voxel" function #1643

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Conversation

sastrys1
Copy link
Contributor

@sastrys1 sastrys1 commented Oct 5, 2023

This analysis command is meant to be used with GIST analysis to estimate water residence time for a given region.

When running GIST on a trajectory, we get a list of voxels and their associated rotational/translational entropy of solvent atoms. We can focus on low-entropy voxels (which behave differently from the bulk) and find the population of solvent atoms in a voxel throughout the course of a simulation to measure how long it's taking for solvent molecules to equilibrate in and out of solvent pockets on the macromolecular interface (survival time correlation function).

"""

lives_in_voxel = []
population = top.atom_indices(mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

use traj.top?

Adding top to function argument was my poor design :(.

import numpy as np


class Test(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're making several PRs, so it would be better to follow the "modern" way to write test, by using pytest's style.

  • The class is not needed
  • Change the name of test_0 to some thing meaningful to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I trigger testing locally? Right now I'm making changes directly to the package that is installed, then running the individual test scripts I'm writing. When I remove the class and if __name__ == __main__ clause and run the script with just the function definitions, the tests themselves do not get executed.

Comment on lines 64 to 65
if __name__ == "__main__":
unittest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

mask="",
voxel_cntr=(0, 0, 0),
voxel_size=5):
"""for a voxel with center xyz and size voxel_size, find atoms that match a given mask
Copy link
Contributor

Choose a reason for hiding this comment

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

"for" -> "For"

@hainm
Copy link
Contributor

hainm commented Oct 5, 2023

@sastrys1 Looks great to me. I have only several minor comments.

@hainm
Copy link
Contributor

hainm commented Oct 5, 2023 via email


# tests to see if function can find an atom if we search with a coordinate
# very close to the atom's xyz coordinates
def search_for_atom(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, for each test, you need to add test_ prefix (test_search_for_atom), otherwise, pytest won't be recognise it.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/Amber-MD/pytraj/actions/runs/6424481585/job/17445206646

e.g: click "Test with pytest" and search for your function's names. They are not in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, got the tests to work by adding the prefix and using your python3 -m pytest command

@hainm
Copy link
Contributor

hainm commented Oct 6, 2023

@sastrys1 here is how github action run the test:

export CPPTRAJHOME=`pwd`/cpptraj && cd tests && pytest -vs --ignore=test_parallel_pmap --ignore=test_run_mpi.py --ignore=test_energy/test_pmap_sander.py --ignore=test_parallel_mpi --ignore=test_actionlist.py

https://github.com/Amber-MD/pytraj/actions

@hainm hainm merged commit 6d62181 into Amber-MD:master Oct 6, 2023
1 check passed
@hainm
Copy link
Contributor

hainm commented Oct 6, 2023

Thanks @sastrys1 very much!

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.

2 participants