Migrate LomapAtomMapper testing to the gufe mixin#157
Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay ! Just left those two comments!
|
|
||
| m2 = LomapAtomMapper.from_dict(d) | ||
|
|
||
| assert m2 |
There was a problem hiding this comment.
Would an isinstance check be better than just the assert?
| "max3d": 999.0, | ||
| "element_change": False, | ||
| "seed": "CC", | ||
| "shift": False, |
There was a problem hiding this comment.
Since we change the default to "False", should this now be testing "True"?
There was a problem hiding this comment.
In this case it's better if it's not the defaults - the roundtrip on the default settings is already taken care of in the underlying mixin. Here I'm just keeping the "old" test, hence the naming containing "different".
There was a problem hiding this comment.
I think I'm probably just missing something, but I was thinking that if we want to test the non defaults here, then it should be "True"?
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , lgtm! (assuming tests passing)
2 participants
Fixes #111