Load and dump TS YAML files in Arkane #1551

alongd · 2019-02-08T17:29:06Z

Motivation

For a more convenient workflow of archiving QM data and easily parsing it again (and soon automating this process and forming a database) it is important to also save and load YAML files of TSs in addition to stable species.

Description of Changes

ArkaneSpecies can now process TSs as well.
So far, a YAML file was generated for stable species iff their structure was given and Thermo() was called (both important data to save)
Now, a TS YAML file is saved iff structures of all relevant reactant/s and product/s were given and Kinetics() was called. We're not saving the kinetics, though.
The RMGObject was changes slightly to save and load lists.
This PR has one small commit not directly related to the topic, changing how Arkane loads energy from QChem files. I could open a different PR if the reviewers prefer.

Testing

An example for a reaction where all species and TS are loaded from YAML files was added (Arkane examples are also used as functional tests).

Reviewer Tips

Run any reaction in Arkane, look at the YAML files generated, then run the reaction again, pointing the TS to the generated YAML file.

codecov · 2019-02-08T18:53:21Z

Codecov Report

Merging #1551 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1551      +/-   ##
==========================================
- Coverage   41.86%   41.84%   -0.02%     
==========================================
  Files         165      165              
  Lines       28008    28008              
  Branches     5713     5713              
==========================================
- Hits        11726    11721       -5     
- Misses      15494    15498       +4     
- Partials      788      789       +1

Impacted Files	Coverage Δ
rmgpy/data/kinetics/family.py	`58.27% <0%> (-0.3%)`	⬇️

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 be58562...07876cf. Read the comment docs.

alongd · 2019-02-28T15:19:45Z

@mliu49, I think this is now ready for review

mliu49

Initial round of comments. I will test it out soon.

mliu49 · 2019-02-28T15:38:47Z

arkane/common.py

@@ -188,11 +188,8 @@ def save_yaml(self, path):
        filename = os.path.join('ArkaneSpecies',
                                ''.join(c for c in self.label if c in valid_chars) + '.yml')
        full_path = os.path.join(path, filename)
-        content = yaml.dump(data=self.as_dict(), Dumper=Dumper)
-        # remove empty lines from the file (multi-line strings have excess new line brakes for some reason):


Yeah... I was worried about this. I probably should have followed up on that.

I looks ugly, but works....
Another option for human input is to use a pipe symbol in yaml, e.g. (in ARC's format):

species: - label: vinoxy smiles: C=C[O] multiplicity: 2 charge: 0 - label: OH xyz: | O 0.00000000 0.00000000 -0.12002167 H 0.00000000 0.00000000 0.85098324 - label: methylamine adjlist: | 1 C u0 p0 c0 {2,S} {3,S} {4,S} {5,S} 2 N u0 p1 c0 {1,S} {6,S} {7,S} 3 H u0 p0 c0 {1,S} 4 H u0 p0 c0 {1,S} 5 H u0 p0 c0 {1,S} 6 H u0 p0 c0 {2,S} 7 H u0 p0 c0 {2,S} - label: propene smiles: C=CC - label: N2H4 smiles: NN

mliu49 · 2019-02-28T15:57:22Z

arkane/qchem.py

                if 'Final energy is' in line:
                    E0 = float(line.split()[3]) * constants.E_h * constants.Na
                    logging.debug('energy is {}'.format(str(E0)))
            if E0 is None:
-                for line in f:


Looking at this again, the reason it fails is because you looping through a file moves a pointer from the beginning to the end. Once you're at the end, looping through again doesn't do anything. Thus, you need to reset the pointer, which can be done using f.seek(0).

However, I think you could probably accomplish this only reading through the file once:

with open (self.path, 'r') as f: a = b = 0 for line in f: if 'Final energy is' in line: a = float(line.split()[3]) * constants.E_h * constants.Na if 'Total energy in the final basis set' in line: b = float(line.split()[8]) * constants.E_h * constants.Na E0 = a or b logging.debug('energy is {}'.format(str(E0)))

Of course the fix you have solves the issue, so it's up to you whether or not to try these other options.

I like your implementation. I'll modify it a bit into
e0 = a if a else b
since I think that if the text for b is in the file, then the text for a is there as well. I'll check and will anyway add a test.
Edit: I just realized that e0 = a or b does exactly that. I'll keep it

mliu49 · 2019-02-28T16:03:02Z

arkane/statmech.py

        _, file_extension = os.path.splitext(path)
        if file_extension in ['.yml', '.yaml']:
-            if TS:


Was this line deleted intentionally?

I think the PR replaces it with is_ts, but probably this commit only captured the deletion. I'll sort the commits out

mliu49 · 2019-02-28T16:35:45Z

rmgpy/rmgobject.pyx

-                    logging.debug("made object {0}".format(class_name))
-                    data[key].append(obj)
+            elif isinstance(val, list) and val:
+                if isinstance(val[0], dict) and 'class' in val[0]:


I think this section could be combined with the first if clause by adding an elif at the second level. Let me know if that made sense to you.

Not so clear. Combine if isinstance(val[0], dict) and 'class' in val[0] with if isinstance(val, dict) and 'class' in val?

Sorry, you can ignore this. I missed the lines that were hidden by GitHub and thought that these changes were part of the same function.

mliu49 · 2019-02-28T16:38:54Z

examples/arkane/reactions/NH2+N2H3=NH+N2H4/N2H3.yml

+chemkin_thermo_string: "N2H3                    H   3N   2          G    10.000  3000.000
+  \ 478.04      1\n 2.42935350E+00 1.15088524E-02-6.01470204E-06 1.59974609E-09-1.70089224E-13
+  \   2\n 2.62991058E+04 1.15861374E+01 4.04880122E+00-4.38083729E-03 5.11833994E-05
+  \   3\n-8.84029667E-08 5.22512137E-11 2.61709988E+04 5.24801815E+00                   4\n"


Do you have any idea why this string is printed differently from the adjlist?

I think it's because the lines are too long, so YAML breaks them, then makes it a multiline string with "...". Perhaps \ns aren't executed then?

alongd · 2019-03-01T15:35:45Z

@mliu49 , I addressed the initial comments, squashed and force-pushed (I re-organized the commits)

mliu49 · 2019-03-01T16:28:17Z

I tested TS dumping and loading, and it seems to work fine.

I added a new commit with a couple changes to yaml dumping. Most notably, I added a custom string representer to make it dump multiline strings as block literals. Please try it out and let me know what you think.

I also removed the Dumper=Dumper argument since it seemed unnecessary. Do you remember why you had added that?

Doing so interfers with reading the line brakes, so whe reading we get: 1 O u0 p2 c0 {2,S} {9,S} 2 C u0 p0 c0 {1,S} {3,S} {4,S} {5,S} 3 C u0 p0 c0 {2,S} {6,S} {7,S} {8,S} 4 H u0 p0 c0 {2,S} 5 H u0 p0 c0 {2,S} 6 H u0 p0 c0 {3,S} 7 H u0 p0 c0 {3,S} 8 H u0 p0 c0 {3,S} 9 H u0 p0 c0 {1,S} instead of: 1 O u0 p2 c0 {2,S} {9,S} 2 C u0 p0 c0 {1,S} {3,S} {4,S} {5,S} 3 C u0 p0 c0 {2,S} {6,S} {7,S} {8,S} 4 H u0 p0 c0 {2,S} 5 H u0 p0 c0 {2,S} 6 H u0 p0 c0 {3,S} 7 H u0 p0 c0 {3,S} 8 H u0 p0 c0 {3,S} 9 H u0 p0 c0 {1,S}

For some reason ARC fails without this modification when sp calculations are done in QChem. It is indeed less efficient, since the entire file is loaded to memory.

Using the 'Total energy in the final basis set' phrase

Some properties are important to parse, but are not objects and cannot be updated automatically. `imaginary_frequency` contains a ScalarQuantity instance, but it is not an object in itself in RMG/Arkane, just an attribute of TransitionState (.frequency). It's important to parse it for tunneling. Structure information is important to parse since YAML file generation depends on it.

Iff structures of all reactant/s and product/s are given

Also, renames `TS` as `is_ts`

alongd · 2019-03-14T18:49:35Z

Thanks for this awesome addition, @mliu49! Now it also looks nice :)
No, I don't remember why I had to add Dumper, I think I just followed what I saw in tutorials, apparently as you show it is not necessary. The TS YAML generation worked fine for me as well. I just rebased, should be good to go.

mliu49 · 2019-03-14T20:07:20Z

Would you like to update the example yaml files following the change to string representation?

Otherwise I think this is ready to merge.

Showing how to use species (and TS) YAML files to run Arkane

Provide file stream directly to yaml.dump Remove optional Dumper argument and associated imports Add str representer to dump multiline strings as block literals

alongd · 2019-03-14T23:47:38Z

Done!

alongd requested a review from mliu49 February 8, 2019 17:29

alongd self-assigned this Feb 8, 2019

alongd added the Arkane label Feb 8, 2019

alongd force-pushed the arkane_ts_yaml branch 2 times, most recently from 4c76ce5 to a815028 Compare February 28, 2019 15:18

mliu49 reviewed Feb 28, 2019

View reviewed changes

alongd force-pushed the arkane_ts_yaml branch from a815028 to d5d37bf Compare March 1, 2019 15:34

alongd added 11 commits March 14, 2019 14:45

Load all lines when reading energy in QChem

1d29a65

For some reason ARC fails without this modification when sp calculations are done in QChem. It is indeed less efficient, since the entire file is loaded to memory.

Added a QChem LoadEnergy test

b1e087a

Using the 'Total energy in the final basis set' phrase

Renamed the species YAML folder to simply species

5469b42

Allow ArkaneSpecies to process TSs as well

ea717d0

Minor: Styling improvements in Arkane kinetics.py

32a0896

Save a YAML file for TSs after computing kinetics

a3aae20

Iff structures of all reactant/s and product/s are given

Allow Statmech to process TSs from YAML

d3eaa30

Also, renames `TS` as `is_ts`

Minor: comments in Arkane thermo

642a3d2

Allow RMGObject to process lists even if they don't contain RMGObjects

c7f9625

alongd force-pushed the arkane_ts_yaml branch from 1bdfbac to ca88d81 Compare March 14, 2019 18:47

alongd and others added 4 commits March 14, 2019 19:46

Added the NH2 + N2H3 = NH + N2H4 Arkane reaction example

e267bdb

Showing how to use species (and TS) YAML files to run Arkane

Modified the H+C2H4=C2H5 Arkane example to save YAML files

d351b81

Modified commonTest according to the recent ArkaneSpecies changes

9b7350c

Minor improvements to yaml dumping for ArkaneSpecies

07876cf

Provide file stream directly to yaml.dump Remove optional Dumper argument and associated imports Add str representer to dump multiline strings as block literals

alongd force-pushed the arkane_ts_yaml branch from ca88d81 to 07876cf Compare March 14, 2019 23:46

mliu49 approved these changes Mar 15, 2019

View reviewed changes

mliu49 merged commit 59aad7a into master Mar 15, 2019

mliu49 deleted the arkane_ts_yaml branch March 15, 2019 16:41

mliu49 mentioned this pull request May 15, 2019

RMG v2.4.0 (?) Release Planning #1591

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load and dump TS YAML files in Arkane #1551

Load and dump TS YAML files in Arkane #1551

alongd commented Feb 8, 2019

codecov bot commented Feb 8, 2019 •

edited

alongd commented Feb 28, 2019

mliu49 left a comment

mliu49 Feb 28, 2019

alongd Feb 28, 2019 •

edited

mliu49 Feb 28, 2019

alongd Feb 28, 2019 •

edited

mliu49 Feb 28, 2019

alongd Feb 28, 2019

mliu49 Feb 28, 2019

alongd Feb 28, 2019

mliu49 Feb 28, 2019

mliu49 Feb 28, 2019

alongd Feb 28, 2019

alongd commented Mar 1, 2019

mliu49 commented Mar 1, 2019

alongd commented Mar 14, 2019

mliu49 commented Mar 14, 2019

alongd commented Mar 14, 2019

Load and dump TS YAML files in Arkane #1551

Load and dump TS YAML files in Arkane #1551

Conversation

alongd commented Feb 8, 2019

Motivation

Description of Changes

Testing

Reviewer Tips

codecov bot commented Feb 8, 2019 • edited

Codecov Report

alongd commented Feb 28, 2019

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd Feb 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd Feb 28, 2019 • edited

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

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

alongd commented Mar 1, 2019

mliu49 commented Mar 1, 2019

alongd commented Mar 14, 2019

mliu49 commented Mar 14, 2019

alongd commented Mar 14, 2019

codecov bot commented Feb 8, 2019 •

edited

alongd Feb 28, 2019 •

edited

alongd Feb 28, 2019 •

edited