Add shift_coord for gro writer #730
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
- Coverage 92.07% 92.06% -0.02%
==========================================
Files 67 67
Lines 6893 6892 -1
==========================================
- Hits 6347 6345 -2
- Misses 546 547 +1
☔ View full report in Codecov by Sentry. |
| @@ -131,7 +134,8 @@ def write_gro(top, filename, precision=3): | |||
|
|
|||
| """ | |||
| pos_array = np.ndarray.copy(top.positions) | |||
| pos_array = _validate_positions(pos_array) | |||
| if shift_coord: | |||
| pos_array = _validate_positions(pos_array) | |||
There was a problem hiding this comment.
I was glancing at this function and feel like it should be rewritten to use vectorized operations instead of this a for loop.
def _validate_positions(pos_array):
"""Modify coordinates, as necessary, to fit limitations of the GRO format."""
if np.min(pos_array) < 0:
warnings.warn(
"Topology contains some negative positions. Translating "
"in order to ensure all coordinates are non-negative."
)
min_xyz = np.min(x, axis=1) # smallest position of each coordinate
min_xyz0 = np.where(min_xyz<0, min_xyz, 0) #shift by 0 or minimum value of the positions in that coordinate
pos_array -= min_xyz0
return pos_arrayThere was a problem hiding this comment.
Same as below comment. I think it's worthwhile to make these changes to this function as well.
There was a problem hiding this comment.
oh shoot, I somehow miss these, I will add these changes in
| @@ -48,6 +48,10 @@ def test_write_gro(self): | |||
| top = from_parmed(pmd.load_file(get_fn("ethane.gro"), structure=True)) | |||
| top.save("out.gro") | |||
|
|
|||
| def test_write_gro_with_shift_coord(self): | |||
| top = from_parmed(pmd.load_file(get_fn("ethane.gro"), structure=True)) | |||
| top.save("out.gro", shift_coord=True) | |||
There was a problem hiding this comment.
read_top = Topology("out.gro")
assert np.all(list(map(lambda x: x.positions>=0, read_top.sites)))
There was a problem hiding this comment.
Not sure how these comments got resolved, @daico007 did you address them somehow?
Co-authored-by: CalCraven <[email protected]>
for more information, see https://pre-commit.ci
…o gro_writer_bug
I bumped into some odd behavior when writing out
grofile, i.e., realizing the coordinates are slightly shifted. I thought it's a bug but turn out to be a feature, i.e., a check to move all sites to ensure their coords are all positive. This PR add the option to preserve the original coordinate for thegrowriter.