Test and fix Molecule.remove_van_der_waals_bonds #1799
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1799 +/- ##
=======================================
Coverage 42.99% 42.99%
=======================================
Files 80 80
Lines 21090 21090
Branches 5513 5513
=======================================
Hits 9067 9067
+ Misses 11008 10995 -13
- Partials 1015 1028 +13
Continue to review full report at Codecov.
|
rwest
added
Complexity: Low
Status: Ready for Review
|
This worked for me. I was getting the following error before while running a reaction with a Pt catalyst: |
|
I am assigning myself as the merger for now to give an opportunity for one of the newer developers to take a look at it if they want the experience. If there is no movement on this by tomorrow evening though I'll go ahead and review/merge this in. |
|
I think this fix is fine, but @mliu49 had implemented something very similar here. Ideally we wouldn't have duplicated code like this, but I don't know if adding this method to the parent class I think the next best thing would be to make these functions consistent, but this isn't a big deal either. @rwest do you mind updating this branch when you get a chance? If you want feel free to make the code consistent between these functions, or if there is a better solution to get rid of duplicating code I am all for that as well. Either way I think this PR is good to go and I'll merge it as soon as it is updated. |
This method currently modifies the dictionary of
bonds while iterating over it. Apparently that
was undetected in Python 2.
Currently this test fails with:
======================================================================
ERROR: Test we can remove a van-der-Waals bond
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/rwest/Code/Cat/RMG-Py/rmgpy/molecule/moleculeTest.py", line 2699, in test_remove_van_der_waals_bonds
mol.remove_van_der_waals_bonds()
File "rmgpy/molecule/molecule.py", line 1091, in rmgpy.molecule.molecule.Molecule.remove_van_der_waals_bonds
def remove_van_der_waals_bonds(self):
File "rmgpy/molecule/molecule.py", line 1097, in rmgpy.molecule.molecule.Molecule.remove_van_der_waals_bonds
for bond in atom.edges.values():
RuntimeError: dictionary changed size during iteration
Subsequent commit will fix it
Fixes this problem:
Error: Problem family: Surface_Adsorption_vdW
Error: Problem reactants: (Molecule(smiles="[Pt]"), Molecule(smiles="CO"))
Traceback (most recent call last):
snip
File "/Users/rwest/Code/Cat/RMG-Py/rmgpy/data/kinetics/family.py", line 1567, in _generate_product_structures
product_structures = self.apply_recipe(reactant_structures, forward=forward)
File "/Users/rwest/Code/Cat/RMG-Py/rmgpy/data/kinetics/family.py", line 1483, in apply_recipe
struct.remove_van_der_waals_bonds()
File "rmgpy/molecule/molecule.py", line 1091, in rmgpy.molecule.molecule.Molecule.remove_van_der_waals_bonds
File "rmgpy/molecule/molecule.py", line 1097, in rmgpy.molecule.molecule.Molecule.remove_van_der_waals_bonds
RuntimeError: dictionary changed
Done in a similar way to the Group method.
|
Thanks for spotting the code duplication. It also annoys me, but I think adding it to "Graph" doesn't make sense, as a Graph should know nothing about whether its 'edges' are van der Waals or not. So I did what you called the "next best thing" and made my fix the same as Max's. While updating the code I spotted a couple of tiny tweaks. Just rebased and pushed. |
Motivation or Problem
There was a bug where
remove_van_der_waals_bondswould modify the dictionary of bonds while iterating over it. I guess Python 3 detects this and crashes, wheras Python 2 didn't.Description of Changes
This pull request adds a failing unit test, then a patch that makes it pass.
Reviewer Tips
For testing, you could run the tests after the first commit (and see them fail) then run them again after the second commit (and see them pass).
Note the test coverage is misleading because this code is cythonized. Coverage should go up, as I added a test.