-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modified the ArkaneSpecies.update_xyz_string() method #1714
Conversation
Codecov Report
@@ Coverage Diff @@
## py3 #1714 +/- ##
=======================================
Coverage 41.41% 41.41%
=======================================
Files 167 167
Lines 30118 30118
Branches 6421 6421
=======================================
Hits 12473 12473
Misses 16685 16685
Partials 960 960 Continue to review full report at Codecov.
|
arkane/common.py
Outdated
xyz_list.append(str(len(self.conformer.number.value_si))) | ||
xyz_list.append(self.label) | ||
for number in self.conformer.number.value_si: | ||
elements.append(getElement(int(number)).symbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for loop necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it's a left-over from a different version of this block
arkane/common.py
Outdated
elements.append(getElement(int(number)).symbol) | ||
for number, coordinate in zip(self.conformer.number.value_si, self.conformer.coordinates.value_si): | ||
element_symbol = getElement(int(number)).symbol | ||
row = element_symbol + ' ' * (4 - len(element_symbol)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use string formatting here: row = '{0:4}'.format(element_symbol)
Note that strings are left aligned by default while numbers are right aligned by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
arkane/common.py
Outdated
for c in coordinate: | ||
row += '{0:14.8f}'.format(c * 1e10) # convert m to Angstrom | ||
xyz_list.append(row) | ||
return os.linesep.join(xyz_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should use linesep here. File writing should automatically take care of line endings (at least in Python 3): https://docs.python.org/3/library/os.html#os.linesep
arkane/common.py
Outdated
element_symbol = getElement(int(number)).symbol | ||
row = element_symbol + ' ' * (4 - len(element_symbol)) | ||
for c in coordinate: | ||
row += '{0:14.8f}'.format(c * 1e10) # convert m to Angstrom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option (although it might be a bit harder to read):
row += '{0:14.8f}{1:14.8f}{2:14.8f}'.format(*(coordinate * 1e10).tolist())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Something's messed up with the coverage report. I don't think this small PR bumped the coverage by 3%... |
Now outputs a standardized form of the xyz, also compatible with Py3 Test extended accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments! Corrected and squashed
arkane/common.py
Outdated
xyz_list.append(str(len(self.conformer.number.value_si))) | ||
xyz_list.append(self.label) | ||
for number in self.conformer.number.value_si: | ||
elements.append(getElement(int(number)).symbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it's a left-over from a different version of this block
arkane/common.py
Outdated
elements.append(getElement(int(number)).symbol) | ||
for number, coordinate in zip(self.conformer.number.value_si, self.conformer.coordinates.value_si): | ||
element_symbol = getElement(int(number)).symbol | ||
row = element_symbol + ' ' * (4 - len(element_symbol)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
arkane/common.py
Outdated
element_symbol = getElement(int(number)).symbol | ||
row = element_symbol + ' ' * (4 - len(element_symbol)) | ||
for c in coordinate: | ||
row += '{0:14.8f}'.format(c * 1e10) # convert m to Angstrom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
59e0701
to
fdf0a48
Compare
Now outputs a standardized form of the xyz
Test extended accordingly
Addresses issue 1 reported in #1713
I think this PR should be merged into the py3 branch before #1713, since it doesn't break Py2 compatibility.