Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2359 +/- ##
===========================================
- Coverage 89.98% 89.97% -0.01%
===========================================
Files 176 176
Lines 22055 22045 -10
Branches 2896 2896
===========================================
- Hits 19846 19836 -10
Misses 1612 1612
Partials 597 597
Continue to review full report at Codecov.
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Removing code is great.
Could you add a regression test?
| .. versionchanged:: 0.20.0 | ||
| Inplace-modification of atom positions is now optional, and positions | ||
| are returned as a numpy array. | ||
| .. versionchanged:: 0.20.2 |
zemanj
left a comment
zemanj
left a comment
There was a problem hiding this comment.
I still don't get the point here. You say that very small boxes can lead to no work being done. The shortcut identifies a molecule as already unwrapped if all distances are smaller than half a box edge length. In other words: the smaller the box, the less likely it should be that no work has to be done.
This comment has been minimized.
This comment has been minimized.
|
@zemanj my review was clearly only scratching the surface whereas you have more in-depth insight. You should turn your last comment into an official review — you have a clear request there and you also clearly don’t want the change to happen until your concerns are addressed. The best way to express this is is a blocking review. Thank you for having a look at the PR! |
zemanj
left a comment
zemanj
left a comment
There was a problem hiding this comment.
I don't understand why the shortcut causes make_whole() to return early if the box is small. I'd really appreciate it you could provide a minimal example reproducing the issue!
| if not is_unwrapped: | ||
| break | ||
| if is_unwrapped: | ||
| return np.array(oldpos) |
3 participants
Fixes #2358
Changes made in this Pull Request:
PR Checklist