Skip to content
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

Add xyz writer #46

Merged
merged 6 commits into from
Feb 25, 2019
Merged

Add xyz writer #46

merged 6 commits into from
Feb 25, 2019

Conversation

mattwthompson
Copy link
Member

Since XYZ files store next to nothing (no residue information, box, bonds, angles, dihedrals) I was able to just enumerate through the sites and avoid the complications Ray is having in #28.

I added an element setter and getter, which may or may or may not affect other behavior but seems like something we should have already done.

The logic in assigning a name could be improved, I just check to see if there's an element stored in the site and if not, I try to assign a "name" according to the first characters of the site's name, and if those fail I just call it X.

@mattwthompson mattwthompson added the enhancement New feature or request label Feb 20, 2019
tmp_name = site.element.symbol
elif len(site.name) <= 2:
tmp_name = site.name
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case when a name is >2? Wouldnt we want to support that as well?

Even though its not an element anymore, i could see a case where your sites are not atomistic representations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This column in XYZ files is supposed to be element. We need better element guessing, but I don't think there are good rules for handling non-elements in this format. For now I guess we can just let the entire name be stored, but it may be problematic down the line - we'll see.

def write_xyz(top, filename):
with open(filename, 'w') as out_file:
out_file.write('{:d}\n'.format(top.n_sites))
out_file.write('{} {} written by topology\n'.format(top.name, filename))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date would be nice to have

@justinGilmer justinGilmer mentioned this pull request Feb 21, 2019
Copy link
Collaborator

@ahy3nz ahy3nz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further down the line, we might want to do some validation of the element


self._element = element
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type checking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer doing this in a separate PR bulking up the element class

@mattwthompson
Copy link
Member Author

I added a simple datetime printer, the result looks like this:

8
Topology tmp.xyz written by topology at 2019-02-24 10:52:34.097767
X    0.000   -1.400   -0.000
X   -1.070   -1.400   -0.000
X    0.357   -2.169    0.653
X    0.357   -1.581   -0.993
X    0.000    0.000    0.000
X    1.070    0.000    0.000
X   -0.357    0.769    0.653
X   -0.357    0.181   -0.993

@ahy3nz ahy3nz self-requested a review February 25, 2019 15:53
@mattwthompson mattwthompson merged commit 393da24 into master Feb 25, 2019
@mattwthompson mattwthompson deleted the write_xyz branch February 28, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants