ur-whitelab / hoomd-tf

A plugin that allows the use of Tensorflow in Hoomd-Blue for GPU-accelerated ML+MD
https://hoomd-tf.readthedocs.io
MIT License
30 stars 8 forks source link

Update mol_{angle,dihedral,bond_distance} to use tf.gather when cg_positions is a tf.Tensor #325

Closed RainierBarrett closed 2 years ago

RainierBarrett commented 2 years ago

This PR is a small optimization to the functions like mol_angle that take in CG indices so they can take in the whole list of CG index pairs at once instead of one pair at a time, when CG_positions is a tensor.

pep8speaks commented 2 years ago

Hello @RainierBarrett! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 512:30: W291 trailing whitespace Line 1006:89: W291 trailing whitespace Line 1007:89: W291 trailing whitespace Line 1008:89: W291 trailing whitespace

Comment last updated at 2021-08-25 17:43:24 UTC
whitead commented 2 years ago

@RainierBarrett you still working on this? Looks like you just need to fix your whitespace.

geemi725 commented 2 years ago

Function updates look good. Will approve when PEP 8 issue is addressed.

RainierBarrett commented 2 years ago

@geemi725 I thought e38da36 was the only pep8 change needed. Is something else missing?

geemi725 commented 2 years ago

Oh it's not shown as fixed for some reason. Good job with PR

whitead commented 2 years ago

Hi @geemi725 and @RainierBarrett, this is still failing due to issue identified by pep8:

 E     File "/usr/share/miniconda/lib/python3.7/site-packages/hoomd/htf/utils.py", line 858
E       if type(cg_positions) == tf.Tensor:
E                                         ^
E   TabError: inconsistent use of tabs and spaces in indentation
RainierBarrett commented 2 years ago

Ahh, I see it was in the test output, not the github bot. I recently set up a new machine and I guess I forgot to configure my editor. I'll address this, give me a little bit.

whitead commented 2 years ago

@RainierBarrett looks MDAnalysis 2.0 version is killing our CI. Can you add MDAnalysis < 2.0 to CI?