Conversation
mattwthompson
changed the title
.gro file.gro files
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1220 +/- ##
=======================================
Coverage 93.85% 93.86%
=======================================
Files 72 72
Lines 6233 6240 +7
=======================================
+ Hits 5850 5857 +7
Misses 383 383 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
This is approved, but it does have the fairly pointy edge case of being vulnerable to different atom orderings. It may be more trouble than it's worth to police this (since truly scrambled atom orderings will lead to massive energies and likely crashes right after) but I think the docstring at least needs to contain this warning in a way that's hard to miss. And I'd be OK with even more extreme cautions, like a UserWarning that's always omitted when the function is used unless the user sets a flag or something (though that's quite extreme so it's not a blocking requirement for merge).
| self, | ||
| gro_file: Path | str, | ||
| ): | ||
| """Set the positions of this `Interchange` from a GROMACS coordinate `.gro` file.""" |
There was a problem hiding this comment.
(blocking) Add a sentence or two here loudly cautioning that this only checks that the NUMBER of atoms matches, and that anything with the right number of atoms will be accepted, even if the actual ordering is scrambled.
2 participants
Description
Beginning #1210
Checklist