Minor: Optional output zmat after modify_coords #405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 54.06% 54.03% -0.03%
==========================================
Files 35 35
Lines 11921 11923 +2
Branches 3696 3697 +1
==========================================
- Hits 6445 6443 -2
- Misses 4505 4508 +3
- Partials 971 972 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@oscarwumit, thank you for your contribution, and I agree with this edition.
arc/species/converter.py
Outdated
| @@ -884,6 +886,8 @@ def modify_coords(coords: Dict[str, tuple], | |||
|
|
|||
| Returns: | |||
| dict: The respective cartesian (xyz) coordinates reflecting the desired modification. | |||
| Returns: | |||
| dict: The internal (zmat) coordinates reflecting the desired modification. | |||
There was a problem hiding this comment.
Minor: @alongd, I am not familiar with the convention. Do we write separate Returns, or we merge them together?
There was a problem hiding this comment.
We should have just one returned type, it could be a composite type, e.g., https://github.com/ReactionMechanismGenerator/ARC/blob/master/arc/species/conformers.py#L739
| if output_zmat: | ||
| return zmat | ||
| else: | ||
| return new_xyz |
There was a problem hiding this comment.
Minor: I suggest to put line 952 into the else section.
|
Thanks for the suggestions. Please take another look. This PR is rebased. @xiaoruiDong |
|
@oscarwumit, can you look at the tests? |
|
@oscarwumit , what's the status of this PR? |
Zmat contains important connectivity information that xyz does not. Giving the option to output zmat instead of xyz after coordinates modification is useful to ensure connectivity information is preserved.
Zmat is particularly useful when
modify_coordsis called multiple times such as changing two dihedrals of a species by calling the function twice. Using xyz in this case may cause bugs as ARC needs to perceive connectivity from xyz twice, and the perceived connectivity might be inconsistent.