-
Notifications
You must be signed in to change notification settings - Fork 233
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Force pushed a small correction to the test. Should be ready for review. |
There was a problem hiding this 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.
rmgpy/data/solvation.py
Outdated
@@ -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): |
There was a problem hiding this comment.
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.
I updated the molecule representation of co-solvent to list. |
Thanks, @mjohnson541, addressed the comment and added a test. |
7dbeb6b
to
43fc6ba
Compare
rmgpy/species.py
Outdated
) | ||
) | ||
|
||
def get_structure(self, structure): |
There was a problem hiding this comment.
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
?
rmgpy/data/solvation.py
Outdated
spc.generate_resonance_structures() | ||
if not isinstance(molecule, list): | ||
molecule = [molecule] | ||
spc = [] |
There was a problem hiding this comment.
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.
Thanks @mliu49 , I addressed the comments in fixup commits |
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. |
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)
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(
... |
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 inSolventLibrary
Testing
Could be tested using -db PR ReactionMechanismGenerator/RMG-database#318