arm61 / pylj

Teaching Utility for Classical Atomistic Simulation.
https://pythoninchemistry.org/pylj
MIT License
29 stars 21 forks source link

Fixed issue with Buckingham not working with dr arrays, added appropr… #47

Closed sansona closed 5 years ago

sansona commented 5 years ago

For #46

Buckingham forcefield was raising a TypeError when dr was an array. Error was in the np.exp() term which was only used in the Buckingham function but not the other forcefields:

np.exp(-constants[1] * dr)

When type(dr) == float, the term inside was fine, but multiplication of a non-int and a float i.e 2.0 * [2.0]isn't allowed in python. I fixed this by converting dr to a numpy array by default. Numpy allows this multiplication in their arrays i.e 2.0 * np.array([2.0]) returns 4.0. The function should now be able to handle both floats and arrays (lists and numpy arrays) fine.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 360


Files with Coverage Reduction New Missed Lines %
pylj/forcefields.py 1 66.0%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 358: 0.0%
Covered Lines: 238
Relevant Lines: 338

💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 359


Changes Missing Coverage Covered Lines Changed/Added Lines %
pylj/forcefields.py 0 5 0.0%
<!-- Total: 0 5 0.0% -->
Totals Coverage Status
Change from base Build 358: -1.9%
Covered Lines: 238
Relevant Lines: 343

💛 - Coveralls
arm61 commented 5 years ago

Sorry for the delay on this (I am really busy between work and viva prep) but would it not be more straight forward to say that the forcefield functions only accept np.ndarray type objects?

sansona commented 5 years ago

No worries at all. It would definitely be. I'm not familiar enough to comment on whether users would attempt arrays as inputs - if that's not the case then I'll add that to the documentation and remove the handling for lists. The other forcefield functions can handle lists so for consistency, I see value in having all the forcefield functions be able to handle the same inputs

arm61 commented 5 years ago

I have moved this over to #48 cause there was a problem with the travis continuous integration and I didn't wanna bog you down with trying to fix it. All of your commits will still be registered and I will merge that pull request once the tests pass!

Thanks again for an awesome contribution :100: :+1: