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 all commits
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
64 changes: 35 additions & 29 deletions arc/species/species.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class ARCSpecies(object):
`ts_guesses` ``list`` A list of TSGuess objects for each of the specified methods
`successful_methods` ``list`` Methods used to generate a TS guess that successfully generated an XYZ guess
`unsuccessful_methods` ``list`` Methods used to generate a TS guess that were unsuccessfully
`chosen_ts` ``int`` The TSGuess index corresponding to the chosen TS conformer used for optimization
`chosen_ts` ``int`` The TSGuess index corresponding to the chosen TS conformer used for
optimization
`chosen_ts_method` ``str`` The TS method that was actually used for optimization
`ts_conf_spawned` ``bool`` Whether conformers were already spawned for the Species (representing a TS)
based on its TSGuess objects
Expand Down Expand Up @@ -195,22 +196,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.warning('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 not self.is_ts:
# Perceive molecule from xyz coordinates
Expand All @@ -227,10 +230,11 @@ def __init__(self, is_ts=False, rmg_species=None, mol=None, label=None, xyz=None
self.number_of_atoms = len(self.mol.atoms)
if self.mol_list is None:
mol_copy = self.mol.copy(deep=True)
self.mol_list = mol_copy.generate_resonance_structures(keep_isomorphic=False, filter_structures=True)
self.mol_list = mol_copy.generate_resonance_structures(keep_isomorphic=False,
filter_structures=True)
elif not self.bond_corrections and self.generate_thermo:
logging.warn('Cannot determine bond additivity corrections (BAC) for species {0} based on xyz'
' coordinates only. For better thermoproperties, provide bond corrections.')
logging.warning('Cannot determine bond additivity corrections (BAC) for species {0} based on xyz'
' coordinates only. For better thermoproperties, provide bond corrections.')

if self.initial_xyz is not None:
# consider the initial guess as one of the conformers if generating others.
Expand Down Expand Up @@ -258,7 +262,7 @@ def __init__(self, is_ts=False, rmg_species=None, mol=None, label=None, xyz=None
if not self.is_ts and self.initial_xyz is None and self.mol is None:
raise SpeciesError('No structure (xyz, SMILES, adjList, RMG:Species, or RMG:Molecule) was given for'
' species {0}'.format(self.label))
if self.label is None :
if self.label is None:
raise SpeciesError('A label must be specified for an ARCSpecies object.')
# Check that `label` is valid, since it is used for folder names
for char in self.label:
Expand Down Expand Up @@ -518,7 +522,7 @@ def find_conformers(self, mol, method='all'):
try:
rd_xyzs, rd_energies = _get_possible_conformers_rdkit(mol)
except ValueError as e:
logging.warn('Could not generate RDkit conformers for {0}, failed with: {1}'.format(
logging.warning('Could not generate RDkit conformers for {0}, failed with: {1}'.format(
self.label, e.message))
if rd_xyzs:
rd_xyz = get_min_energy_conformer(xyzs=rd_xyzs, energies=rd_energies)
Expand Down Expand Up @@ -567,8 +571,8 @@ def set_dihedral(self, scan, pivots, deg_increment):
`pivots` is used to identify the rotor.
"""
if deg_increment == 0:
logging.warning('set_dihedral was called with zero increment for {label} with pivots {pivots}'.format(
label=self.label, pivots=pivots))
logging.warning('set_dihedral was called with zero increment for {label} with pivots {pivots}'
.format(label=self.label, pivots=pivots))
for rotor in self.rotors_dict.values(): # penalize this rotor to avoid inf. looping
if rotor['pivots'] == pivots:
rotor['times_dihedral_set'] += 1
Expand Down Expand Up @@ -633,14 +637,14 @@ def determine_symmetry(self):
optical_isomers = 2 if pg.chiral else optical_isomers
self.optical_isomers = self.optical_isomers if self.optical_isomers is not None else optical_isomers
if self.optical_isomers != optical_isomers:
logging.warn("User input of optical isomers for {0} and ARC's calculation differ: {1} and {2},"
" respectively. Using the user input of {1}".format(
self.label, self.optical_isomers, optical_isomers))
logging.warning("User input of optical isomers for {0} and ARC's calculation differ: {1} and {2},"
" respectively. Using the user input of {1}"
.format(self.label, self.optical_isomers, optical_isomers))
self.external_symmetry = self.external_symmetry if self.external_symmetry is not None else symmetry
if self.external_symmetry != symmetry:
logging.warn("User input of external symmetry for {0} and ARC's calculation differ: {1} and {2},"
" respectively. Using the user input of {1}".format(
self.label, self.external_symmetry, symmetry))
logging.warning("User input of external symmetry for {0} and ARC's calculation differ: {1} and {2},"
" respectively. Using the user input of {1}"
.format(self.label, self.external_symmetry, symmetry))

def determine_multiplicity(self, smiles, adjlist, mol):
if mol:
Expand Down Expand Up @@ -827,6 +831,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 Expand Up @@ -952,12 +958,12 @@ def from_dict(self, ts_dict):
self.family = ts_dict['family'] if 'family' in ts_dict else None
if self.family is None and self.method.lower() in ['kinbot', 'autotst']:
# raise TSError('No family specified for method {0}'.format(self.method))
logging.warn('No family specified for method {0}'.format(self.method))
logging.warning('No family specified for method {0}'.format(self.method))
self.rmg_reaction = ts_dict['rmg_reaction'] if 'rmg_reaction' in ts_dict else None
if self.rmg_reaction is not None:
plus = ' + '
arrow = ' <=> '
if not arrow in self.rmg_reaction:
if arrow not in self.rmg_reaction:
raise TSError('Could not read the reaction string. Expected to find " <=> ". '
'Got: {0}'.format(self.rmg_reaction))
sides = self.rmg_reaction.split(arrow)
Expand Down