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.

Sign up for GitHub

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

BugFix: Don't overwrite self.mol if self.mol_from_xyz is None #106

Merged
merged 2 commits into from
Mar 28, 2019
Merged

BugFix: Don't overwrite self.mol if self.mol_from_xyz is None #106

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
277e7ef
BugFix: Don't overwrite self.mol if self.mol_from_xyz is None
oscarwumit Mar 26, 2019
f9e42b2
Replace logging.warn to logging.warning
oscarwumit Mar 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions arc/species/species.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,22 +195,24 @@ def __init__(self, is_ts=False, rmg_species=None, mol=None, label=None, xyz=None
self.mol.toSMILES(), self.label))
self.multiplicity = self.rmg_species.molecule[0].multiplicity
self.charge = self.rmg_species.molecule[0].getNetCharge()
else:
# parameters were entered directly, not via an RMG:Species object

if label is not None:
self.label = label
if self.mol is None:
if adjlist:
self.mol = Molecule().fromAdjacencyList(adjlist=adjlist)
elif inchi:
self.mol = rmg_mol_from_inchi(inchi)
elif smiles:
self.mol = Molecule(SMILES=smiles)
if not self.is_ts and self.mol is None and self.generate_thermo:
logging.warn('No structure (SMILES, adjList, RMG:Species, or RMG:Molecule) was given for species'
' {0}, NOT using bond additivity corrections (BAC) for thermo computation.'.format(
self.label))
if multiplicity is not None:
self.multiplicity = multiplicity
if charge is not None:
self.charge = charge
if self.mol is None:
if adjlist:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add mol before adjList, inchi and SMILES (it can also be directly given)

Copy link
Contributor Author

@oscarwumit oscarwumit Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we already have self.mol = mol in the init? See line 118, species.py @alongd

Copy link
Member

Choose a reason for hiding this comment

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

good point

self.mol = Molecule().fromAdjacencyList(adjlist=adjlist)
elif inchi:
self.mol = rmg_mol_from_inchi(inchi)
elif smiles:
self.mol = Molecule(SMILES=smiles)
if not self.is_ts and self.mol is None and self.generate_thermo:
logging.warn('No structure (SMILES, adjList, RMG:Species, or RMG:Molecule) was given for species'
Copy link
Member

Choose a reason for hiding this comment

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

Please change logging.warn (which will be deprecated soon) to logging.warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made another commit to replace all logging.warn to logging.warning in species.py. I also changed the code to conform to PEP 8.

' {0}, NOT using bond additivity corrections (BAC) for thermo computation.'.format(
self.label))

if not self.is_ts:
# Perceive molecule from xyz coordinates
Expand Down Expand Up @@ -827,6 +829,8 @@ def mol_from_xyz(self, xyz=None):
'Got xyz:\n{0}\n\nwhich corresponds to {1}\n{2}\n\nand: {3}\n{4}'.format(
xyz, self.mol.toSMILES(), self.mol.toAdjacencyList(),
original_mol.toSMILES(), original_mol.toAdjacencyList()))
if self.mol is None:
self.mol = original_mol # todo: Atom order will not be correct, need fix
else:
self.mol = molecules_from_xyz(xyz, multiplicity=self.multiplicity, charge=self.charge)[1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alongd What if after the else statement, molecules_from_xyz gives None. In this case, it seems that we will still have a problem of self.mol = None. How should we resolve that?

Copy link
Member

Choose a reason for hiding this comment

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

If this condition is true, then this means that self.mol is None to begin with, and molecules_from_xyz is our last resort. If it doesn't work, the user is expected to give some 2D representation. When we do it in an automated fashion, we have an RMG Species object, so it shouldn't be a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.


Expand Down