Fix solvaion radical bug #1773
Fix solvaion radical bug #1773
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1773 +/- ##
=======================================
Coverage 43.98% 43.98%
=======================================
Files 83 83
Lines 21581 21581
Branches 5653 5653
=======================================
Hits 9492 9492
+ Misses 11043 11029 -14
- Partials 1046 1060 +14
Continue to review full report at Codecov.
|
| species = Species(molecule=[Molecule(smiles='[OH]')]) | ||
| solute_data = self.database.get_solute_data_from_groups(species) | ||
| self.assertTrue('radical' in solute_data.comment) | ||
|
|
There was a problem hiding this comment.
Just a thought, should we add a test to check if we get the same (we shouldn't) Abraham solute parameters for OH radical and water? If we get the same values then we know something is wrong.
There was a problem hiding this comment.
I could add that test. I will just add an additional test for checking whether their A parameters are different
|
Thanks for the PR. I add a minor comment. |
|
What's the status on this PR? |
|
@mliu49 I'll make the change and force-push this PR again!! |
yunsiechung
force-pushed
the
fix_solvaion_radical_bug
branch
from
bc182fd to
5c007e7
Compare
|
@oscarwumit I added another unittest that you suggested before. Could you review it and approve it if it's okay? |
rmgpy/data/solvationTest.py
Outdated
| solvent_data = self.database.get_solvent_data('water') | ||
| rad_solvation_correction = self.database.get_solvation_correction(rad_solute_data, solvent_data) | ||
| sat_solvation_correction = self.database.get_solvation_correction(sat_solute_data, solvent_data) | ||
| self.assertNotEqual(rad_solvation_correction.gibbs / 1000, sat_solvation_correction.gibbs / 1000) |
There was a problem hiding this comment.
You may want to use assertNotAlmostEqual here since you're comparing (I assume) floats which have gone through some calculations.
There was a problem hiding this comment.
Thanks for the comment! I applied this change
rmgpy/data/solvationTest.py
Outdated
| def test_radical_solute_group(self): | ||
| """Test that the existing radical group is found for the radical species when using group additivity""" | ||
| # First check whether the radical group is found for the radical species | ||
| rad_species = Species(molecule=[Molecule(smiles='[OH]')]) |
There was a problem hiding this comment.
You can simplify this species creation to Species(smiles='[OH]').
There was a problem hiding this comment.
I applied this change!
yunsiechung
force-pushed
the
fix_solvaion_radical_bug
branch
from
5c007e7 to
7bddcb0
Compare
|
Thanks! Could you rebase? |
The solvation group additivty method had a bug that it cannot apply radical correction to the radical species even if its radical group exists in solvation radical database. It was because originally, when it was applying correction for each radical found, the for-loop range started from range(1, number of radical), instead of starting from range(0, number of radical). Because of that, if the number of radical is 1, the correction was never applied. This commits fixes this error by letting the range to be just range(number of radical) since the number of radical is always integer
I added one more unit test in solvationTest.py to make sure that the radical group is found for the radical species if its radical group exists in the solvation radical database
Another uniitest is added to compare the radical and its saturated species solvation free energy in water. The test makes sure the two solvation energies are different.
yunsiechung
force-pushed
the
fix_solvaion_radical_bug
branch
from
7bddcb0 to
8e93013
Compare
|
@mliu49 I just rebased and pushed again! |
Motivation or Problem
Solvation correction for radical groups were not applied for radical species even if their radical groups were found in the solvation radical database.
Description of Changes
Before, when the radical solvation correction would apply, it applied the correction for each individual radical electron found by using such for loop:
for electron in range(1, atom.radical_electrons):But this was wrong, because the range should start from 0, not 1. If the number of radical electron is 1, then the range is from (1,1), which doesn't really start the for loop at all.
I changed this for loop to:
for electron in range(atom.radical_electrons):such that if the number of electron is 1, the range will be from [0, 1], making the correction to be applied once. Since the number of radical electrons are always 0 or positive integer, this change should be appropriate.
Testing
I also added a unit test in solvationTest.py, which checks whether the solvation radical group is found for the radical speices '[OH]' when calculating its solute data from group additivity method.
Reviewer Tips
fixes #1743