Closed donerancl closed 7 months ago
@JacksonBurns I don't have any new insights on why certain fragments cause this bug, but I changed the generate_kekule_structures
function to return a deep copy of the input molecule if kekulize(molecule)
raises an error. The docstring for the kekulize
function in kekulize.py
warns that "If the molecule cannot be kekulized, a KekulizationError will be raised. However, the molecule will be left in a semi-kekulized state. Therefore, if the original molecule needs to be kept, it is advisable to create a copy before kekulizing." I think this change would be okay though, since I am using returning the original copy instead of the semi-kekulized molecule. Do you think we should make this change before solving the bug?
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Attention: Patch coverage is 0%
with 2 lines
in your changes are missing coverage. Please review.
Project coverage is 54.85%. Comparing base (
7519944
) to head (785e739
). Report is 2 commits behind head on main.:exclamation: Current head 785e739 differs from pull request most recent head d1c38d3. Consider uploading reports for the commit d1c38d3 to get more accurate results
Files | Patch % | Lines |
---|---|---|
rmgpy/molecule/resonance.py | 0.00% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hmm so at face value this looks reasonable, and all of the tests pass...
kekulize
is in two places in RMG, the molecule.kekulize
method and the kekulize
function in kekulize.pyx
- the former simply calls the latter on itself.
I looked around the codebase for other uses of kekulize
and found this: https://github.com/ReactionMechanismGenerator/RMG-Py/blob/a458b1deb3c26f69cff627396e3a635538c1cff6/rmgpy/data/transport.py#L404
which makes it seem like, in other cases, if kekulization fails we expect it to just throw the error and stop execution.
This generate_kekule_structure
function is the only place where we actually catch and deal with the error.
Overall I am somewhat unsure if this is an acceptable change.. @hwpang could you take a look at this?
I think I found the deeper issue. generate_kekule_structures
loops through atoms and checks for benzene/fused benzene atoms. If it finds one, it immediately tries kekulization. The error arises when generate_kekule_structures
checks the atom.atomtype.label
of a CuttingLabel
and finds None
.
I think this was intended to be addressed by checking whether atom
is an instance of Atom
, but RMG thinks the CuttingLabel
is an Atom
.
Ok, I fixed the check for CuttingLabel
s in generate_kekule_structures
, but any input on whether it is good practice is welcome @JacksonBurns
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
This looks good! Could you squash these commits into one (since some of them are just undoing the previous ones) and then re-request a review? Thanks!
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Begins to address Issue #2642
Motivation or Problem
A clear and concise description of what what you're trying to fix or improve. Please reference any issues that this addresses.
Description of Changes
A clear and concise description of what what you've changed or added.
Testing
A clear and concise description of testing that you've done or plan to do.
Reviewer Tips
Suggestions for verifying that this PR works or other notes for the reviewer.