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

oscarwumit · 2019-03-26T20:00:29Z

Temporary fix. Need to troubleshoot why self.mol_from_xyz is None
i.e. why it does not parse xyz as expected

codecov · 2019-03-26T21:03:11Z

Codecov Report

Merging #106 into master will decrease coverage by 9.1%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
- Coverage    40.6%   31.49%   -9.11%     
==========================================
  Files          22       22              
  Lines        5221     5226       +5     
  Branches     1353     1357       +4     
==========================================
- Hits         2120     1646     -474     
- Misses       2759     3271     +512     
+ Partials      342      309      -33

Impacted Files	Coverage Δ
arc/species/species.py	`35.15% <0%> (-23.25%)`	⬇️
arc/species/converter.py	`24.78% <0%> (-47.01%)`	⬇️
arc/species/xyz_to_2d.py	`16.61% <0%> (-22.16%)`	⬇️
arc/scheduler.py	`14.44% <0%> (-5.06%)`	⬇️
arc/reaction.py	`38.58% <0%> (-3.55%)`	⬇️
arc/main.py	`40.36% <0%> (-1.68%)`	⬇️
arc/job/job.py	`20.95% <0%> (-1.11%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e1638b...4c4dce8. Read the comment docs.

codecov · 2019-03-26T21:03:12Z

Codecov Report

Merging #106 into master will increase coverage by 0.01%.
The diff coverage is 39.13%.

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage    40.6%   40.62%   +0.01%     
==========================================
  Files          22       22              
  Lines        5221     5226       +5     
  Branches     1353     1357       +4     
==========================================
+ Hits         2120     2123       +3     
+ Misses       2759     2758       -1     
- Partials      342      345       +3

Impacted Files	Coverage Δ
arc/species/species.py	`58.4% <39.13%> (ø)`	⬆️
arc/reaction.py	`42.12% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e1638b...f9e42b2. Read the comment docs.

oscarwumit · 2019-03-26T21:13:28Z

arc/species/species.py

@@ -827,6 +830,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]


@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?

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

alongd

Thanks! please see comments

alongd · 2019-03-27T00:14:04Z

arc/species/species.py

-            else:
-                # parameters were entered directly, not via an RMG:Species object
+
+            # parameters were entered directly, not via an RMG:Species object


This comment should be removed, since the code here refers to all cases now

alongd · 2019-03-27T00:16:04Z

arc/species/species.py

                self.charge = charge
+            if self.mol is None:
+                if adjlist:


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

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

alongd · 2019-03-27T00:20:01Z

arc/species/species.py

@@ -827,6 +830,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]


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

alongd · 2019-03-27T00:22:51Z

Thanks for your contribution!
Regarding the commit message, I prefer to have a BugFix in the beginning, this capitalization is easier to spot when briefly looking at a bunch of commits. Also, in the commit message, please replace = None with is None.

oscarwumit · 2019-03-27T05:30:55Z

@alongd Many thanks for your review. I have changed the commit message. Please let me what do you think of the self.mol = mol in the init.

alongd · 2019-03-27T09:46:23Z

Could you change the commit message (not the PR title)?

Temporary fix. Need to troubleshoot why self.mol_from_xyz is None i.e. why it does not parse xyz as expected

oscarwumit · 2019-03-27T14:39:25Z

@alongd I updated the commit message and also rebased.

alongd · 2019-03-27T18:15:31Z

arc/species/species.py

+                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'


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

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.

alongd · 2019-03-27T18:17:22Z

I added another small comment.
Also, the commit massage says "temp, need to see why we don't get the correct xyz...", so please open an issue and give the details of the species we saw the bug for, so we'll remember to deal with it.

Also modified code to conform PEP 8 style guide

oscarwumit · 2019-03-27T19:03:24Z

Many thanks for the comment. I opened an issue addressing our problem here.

alongd

Thanks!!

oscarwumit requested a review from alongd March 26, 2019 20:00

oscarwumit force-pushed the fix_parsing_molecule branch from 4c4dce8 to 280b4eb Compare March 26, 2019 21:10

oscarwumit commented Mar 26, 2019

View reviewed changes

alongd reviewed Mar 27, 2019

View reviewed changes

oscarwumit force-pushed the fix_parsing_molecule branch from 280b4eb to 345324a Compare March 27, 2019 05:23

oscarwumit changed the title ~~bug fix: avoid overwriting self.mol if self.mol_from_xyz = None~~ BugFix: avoid overwriting self.mol if self.mol_from_xyz is None Mar 27, 2019

oscarwumit changed the title ~~BugFix: avoid overwriting self.mol if self.mol_from_xyz is None~~ BugFix: avoid overwrite self.mol if self.mol_from_xyz is None Mar 27, 2019

oscarwumit changed the title ~~BugFix: avoid overwrite self.mol if self.mol_from_xyz is None~~ BugFix: Don't overwrite self.mol if self.mol_from_xyz is None Mar 27, 2019

alongd mentioned this pull request Mar 27, 2019

Allow ARCSpecies to be initialized with a conformer list file #104

Merged

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

277e7ef

Temporary fix. Need to troubleshoot why self.mol_from_xyz is None i.e. why it does not parse xyz as expected

oscarwumit force-pushed the fix_parsing_molecule branch from 345324a to 277e7ef Compare March 27, 2019 14:38

alongd reviewed Mar 27, 2019

View reviewed changes

Replace logging.warn to logging.warning

f9e42b2

Also modified code to conform PEP 8 style guide

alongd approved these changes Mar 28, 2019

View reviewed changes

alongd merged commit 8b5db7a into master Mar 28, 2019

alongd deleted the fix_parsing_molecule branch March 28, 2019 02:02

oscarwumit mentioned this pull request Mar 28, 2019

self.mol_from_xyz is None #107

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oscarwumit commented Mar 26, 2019 •

edited

Loading

codecov bot commented Mar 26, 2019

codecov bot commented Mar 26, 2019 •

edited

Loading

oscarwumit Mar 26, 2019

alongd Mar 27, 2019

oscarwumit Mar 27, 2019

alongd left a comment

alongd Mar 27, 2019

oscarwumit Mar 27, 2019

alongd Mar 27, 2019

oscarwumit Mar 27, 2019 •

edited

Loading

alongd Mar 27, 2019

alongd Mar 27, 2019

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

alongd Mar 27, 2019

oscarwumit Mar 27, 2019

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

alongd left a comment

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

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

Conversation

oscarwumit commented Mar 26, 2019 • edited Loading

codecov bot commented Mar 26, 2019

Codecov Report

codecov bot commented Mar 26, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarwumit Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented Mar 27, 2019

oscarwumit commented Mar 27, 2019

alongd left a comment

Choose a reason for hiding this comment

oscarwumit commented Mar 26, 2019 •

edited

Loading

codecov bot commented Mar 26, 2019 •

edited

Loading

oscarwumit Mar 27, 2019 •

edited

Loading