Add function to calculate total vhf from partial vhf - Githubissues

mattwthompson / scattering

Functions for analyzing molecular simulations according to scattering experiments

MIT License

9 stars 9 forks source link

Add function to calculate total vhf from partial vhf #45

Closed lisankim0321 closed 3 years ago

lisankim0321 commented 3 years ago

Sums up partial vhf to calculate the total vhf

rmatsum836 commented 3 years ago

@lisankim0321 Could you go ahead and add your unit tests to test_van_hove.py as well?

rmatsum836 commented 3 years ago

Could you also run black on your code:

conda install -c conda-forge black
black your_file.py

rmatsum836 commented 3 years ago

Made a couples changes:

Renamed the variable in the loop from atoms to atom_pair since you already named a variable atoms.
Removed a check looking for "-" in atom_pair. The above check should handle this case
I added a test to check if the ValueErrors are successfully raised. The pytest.mark.parametrize allows you to added multiple test cases in a single test. Could you add another case so that all checks are covered by tests?

rmatsum836 commented 3 years ago

I'm going to go ahead and mark this PR as ready for review.

rmatsum836 commented 3 years ago

Just fixed merge conflicts with master

rmatsum836 commented 3 years ago

@lisankim0321 When you have some time, could you look into why the results from compute_van_hove and compute_partial_van_hove don't exactly match up?

@JacobLumpkins could you also review this PR? Thanks!