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.

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

Allow cosolvents to be defined in a solvent library #1558

Merged
merged 3 commits into from
Apr 27, 2019
Merged

Conversation

alongd
Copy link
Member

@alongd alongd commented Mar 4, 2019

Motivation or Problem

We need to define a mixture of solvents in the solvent library

Description of Changes

We now parse both a list of descriptors or just a descriptor as a solvent molecule. The descriptors could be either SMILES or adjLists.
A new method, get_structure was created in SolventLibrary

Testing

Could be tested using -db PR ReactionMechanismGenerator/RMG-database#318

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #1558 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
- Coverage   41.61%   41.58%   -0.04%     
==========================================
  Files         176      176              
  Lines       29138    29147       +9     
  Branches     5991     5995       +4     
==========================================
- Hits        12127    12120       -7     
- Misses      16172    16187      +15     
- Partials      839      840       +1
Impacted Files Coverage Δ
rmgpy/species.py 0% <0%> (ø) ⬆️
rmgpy/rmg/main.py 22.15% <100%> (+0.06%) ⬆️
rmgpy/data/solvation.py 64.46% <100%> (-0.25%) ⬇️
rmgpy/data/kinetics/family.py 52.74% <0%> (-0.24%) ⬇️

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 69b3b4b...cd9d91a. Read the comment docs.

@alongd
Copy link
Member Author

alongd commented Mar 5, 2019

Force pushed a small correction to the test. Should be ready for review.
@oscarwumit, could you change the corresponding database PR to have list representations so we could check this branch?

Copy link
Contributor

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

Mostly just some comments on the new function you added. I think we should add a test demonstrating the loading of cosolvents before merging this.

@@ -324,7 +320,22 @@ def getSolventStructure(self, label):
Get a solvent's molecular structure as SMILES or adjacency list from its name
"""
return self.entries[label].item


def get_structure(self, label, molecule):
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the desire to reduce code duplication, but this really shouldn't be a bound function of SolventLibrary. I suggest adding a similar function to species.py or just having the code duplication.

@oscarwumit
Copy link
Contributor

Force pushed a small correction to the test. Should be ready for review.
@oscarwumit, could you change the corresponding database PR to have list representations so we could check this branch?

I updated the molecule representation of co-solvent to list.

@alongd
Copy link
Member Author

alongd commented Mar 15, 2019

Thanks, @mjohnson541, addressed the comment and added a test.
(reorganized commits and force pushed)

@alongd alongd self-assigned this Mar 17, 2019
@alongd alongd force-pushed the cosolvent branch 2 times, most recently from 7dbeb6b to 43fc6ba Compare March 17, 2019 19:46
rmgpy/species.py Outdated
)
)

def get_structure(self, structure):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should be called set_structure or set_molecule?

spc.generate_resonance_structures()
if not isinstance(molecule, list):
molecule = [molecule]
spc = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name this variable something like spcs or spc_list for consistency with expectations.

@alongd
Copy link
Member Author

alongd commented Apr 27, 2019

Thanks @mliu49 , I addressed the comments in fixup commits

@mliu49
Copy link
Contributor

mliu49 commented Apr 27, 2019

Thanks! You can squash and rebase. This PR looks ok to me code-wise, though I'm not fully aware of the context for this change. It seems like @mjohnson541's comments have been resolved, so I think it could be merged if you think it's ready.

alongd added 3 commits April 27, 2019 13:20
To get the molecule list from either SMILES or adjList
`item` is now a list of species (even if there's only one species of
solvent)
@alongd
Copy link
Member Author

alongd commented Apr 27, 2019

Thanks! Squashed and pushed.

The context is that we'd like to define a binary solvent mixture, e.g. 50% water and 50% methanol. We can get Abraham parameters from the mixture from literature (ReactionMechanismGenerator/RMG-database#318), but the solvent library forced us to define only one molecular structure for the solvent. Merging this PR will allow us to define something like:

entry(
    index = 27,
    label = "methanol_50_water_50",
    molecule = ["CO", "O"],
    solvent = SolventData(
    ...

@mliu49 mliu49 dismissed mjohnson541’s stale review April 27, 2019 20:24

Comments have been addressed

@mliu49 mliu49 merged commit bf2d163 into master Apr 27, 2019
@mliu49 mliu49 deleted the cosolvent branch April 27, 2019 20:24
@mliu49 mliu49 mentioned this pull request May 15, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants