Skip to content

Standardize model chemistry#1940

Merged
amarkpayne merged 18 commits into
masterfrom
std_modelchem
May 21, 2020
Merged

Standardize model chemistry#1940
amarkpayne merged 18 commits into
masterfrom
std_modelchem

Conversation

@cgrambow

@cgrambow cgrambow commented May 6, 2020

Copy link
Copy Markdown

Motivation or Problem

Model chemistry definitions in Arkane are not unique because changes in software and options can lead to different results. The goal is to standardize the representation in Arkane and in the RMG database by defining detailed and unique levels of theory to identify the settings used in quantum chemistry calculations.

Description of Changes

Implements two new classes, LevelOfTheory and CompositeLevelOfTheory, which are used by Arkane internally instead of model chemistry strings. Arkane input files can still be specified using the modelChemistry syntax, but using the new level of theory objects is now also possible. Arkane will first try to find an exact match for the specified level of theory in the RMG database (including options like software, solvation, and additional arguments) but will fall back to just matching method and basis if an exact match is not possible (and will print a warning if this is the case).

The corresponding database PR ReactionMechanismGenerator/RMG-database#409 updates the database to also use the new level of theory objects. This required some changes to RMGObject to enable using hashable objects as dictionary/YAML keys.

Testing and Reviewer Suggestions

Unit tests were adapted and should pass. Make some Arkane input files using the new syntax and check that it works as expected.

@cgrambow cgrambow added Arkane Status: WIP This is currently work-in-progress labels May 6, 2020
@cgrambow cgrambow self-assigned this May 6, 2020
@codecov

codecov Bot commented May 6, 2020

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@74cee80). Click here to learn what that means.
The diff coverage is n/a.

@alongd alongd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for pulling this off so quickly! I understand it's WIP. I Added two minor comments

Comment thread arkane/modelchem.py Outdated
Comment thread arkane/modelchem.py Outdated
@lgtm-com

lgtm-com Bot commented May 8, 2020

Copy link
Copy Markdown

This pull request introduces 2 alerts when merging 7fe83c3 into 99d009a - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a class instantiation

@alongd

alongd commented May 9, 2020

Copy link
Copy Markdown
Member

A possible additional attribute to add to the level of theory is solvation.
This is what I have from TCKDB:

solvation (Dict[str, str]): The solvation method used. Keys (values) are:
    - 'method' (str): The solvation method used, e.g., 'SMD'.
    - 'solvent' (str): The considered solvent, e.g., 'water'.
    - 'description' (str): An optional description of the solvation method used if not standard.

@cgrambow cgrambow force-pushed the std_modelchem branch 2 times, most recently from 8b278b3 to 1334dbe Compare May 11, 2020 20:50
@cgrambow cgrambow requested review from alongd and amarkpayne May 11, 2020 20:50
@cgrambow cgrambow added Status: Ready for Review PR is complete and ready to be reviewed and removed Status: WIP This is currently work-in-progress labels May 11, 2020
@cgrambow

Copy link
Copy Markdown
Author

Thanks for the initial comments @alongd. This is now ready for review.

@amarkpayne amarkpayne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code itself looks really good. I have only a few comments code wise. I might have a few more comments about the big picture--we can discuss offline at our meeting today after I have thought about it some more.

We should also figure out how we want to handle the cases where the software is required (you have the check for making sure that the user specifies it, but how should this affect warning messages/behavior when the software does not match?)

Comment thread arkane/modelchem.py
Comment thread arkane/modelchem.py
Comment thread arkane/modelchem.py Outdated
Comment thread arkane/modelchem.py Outdated
Comment thread rmgpy/rmgobject.pyx Outdated
Comment thread arkane/encorr/corr.py
Comment thread arkane/encorr/corr.py
Comment thread arkane/statmech.py
Comment thread examples/arkane/species/Pentyl_isodesmic/input.py Outdated
Comment thread arkane/modelchem.py Outdated
@cgrambow cgrambow force-pushed the std_modelchem branch 5 times, most recently from 81837c8 to 55d7a96 Compare May 20, 2020 12:54
Standardize representation of quantum chemistry levels of theory by
using hashable classes.
Colin Grambow added 17 commits May 20, 2020 14:55
Throw an error if not and include software in the simple representation.
Hashable class instances can act as dictionary keys. Allow such
instances to be created when recreating objects from a dictionary
representation.
Isodesmic calculations require matching the software exactly, so update
the Arkane input file to use LevelOfTheory with software.
@cgrambow cgrambow force-pushed the std_modelchem branch 2 times, most recently from 467e266 to eb3e4ff Compare May 21, 2020 20:30
@amarkpayne amarkpayne merged commit 2968e50 into master May 21, 2020
@amarkpayne amarkpayne deleted the std_modelchem branch May 21, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arkane Status: Ready for Review PR is complete and ready to be reviewed Twin RMG-database PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants