search

coarse-graining / cgnet

learning coarse-grained force fields
BSD 3-Clause "New" or "Revised" License
57 stars 26 forks source link

added to Angstrom unit check logic and typo fix #178

Closed nec4 closed 4 years ago

nec4 commented 4 years ago

Development:

Heyo - while testing out some custom repulsions for backbone atoms like CA and O, I noticed that the angstrom unit conversion check in calculate_hard_sphere_minima() specifies Angstroms but not angstroms, while just above this a value error is raised only for angstroms or nanometers. I'm not sure if this is a case where the user should be expected to read the docs and explicitly specify Angstroms. The unit conversion would originally not occur if 'angstroms' was specified, but the user would not be warned with a ValueError because the check above it would also pass. I don't know if this is important to fix, or if this is even the best way to do it, so we can discuss of course. If this not a needed fix then we can also just close this PR/branch too!

The only other thing is just a typo I found in one of the test names in test_trajectory.py.

brookehus commented 4 years ago

What do you think of my change in 3d154d9 instead?

nec4 commented 4 years ago

3d154d9 looks even cleaner - I like it! Other than this, I have nothing else to add, so if you are happy I await the LGTM - if there is anything else to change, let me know!