RMG-Py
RMG-Py copied to clipboard
ReactionMechanismGenerator
Reimplement Clar Optimization with Scipy MILP
Motivation or Problem
Clar optimization was originally implemented with lpsolve55, potentially introducing difficulties in maintenance. This PR replaces lpsolve55 with scipy.optimize.milp with a hope to alleviate the burden in maintaining this external package.
Description of Changes
- Replace lpsolve APIs with the scipy milp APIs. The new implementation potentially has a slightly better performance (due to vectorization, less data transfer, comparable solver performance, etc.) and improved readability.
- Decouple the MILP solving step (as _solve_clar_milp ) from the MILP formulation step. The motivation is to avoid unnecessary computation. The original approach includes molecule analysis (specifically
get_aromatic_rings) into the recursive calls. However, this is not necessary, as molecules are just copied over and not modified at all. Therefore, analyzing once is enough.
Testing
No new tests are added. The replacement expects no failure in previous tests.
Reviewer Tips
A short meeting with me @xiaoruiDong before the reviewing is preferred, which should help the reviewer understand changes.
A worthy goal - thanks for this PR. Removing dependencies is always a good thing! Will removing lpsolve55 from all the CI, environment specification, installation instructions, etc. etc. come as a follow-up PR?
Thank you for the PR! Do we have a test for the clar optimization to test this part of code?
Yes, there's the Clar test covering relevant topics. https://github.com/ReactionMechanismGenerator/RMG-Py/blob/04559705742d3948712526427cfd60106d21fae6/test/rmgpy/molecule/resonanceTest.py#L1367
And by default during the resonance structure generation for PAHs, RMG will also run clar optimization every time.
A worthy goal - thanks for this PR. Removing dependencies is always a good thing! Will removing
lpsolve55from all the CI, environment specification, installation instructions, etc. etc. come as a follow-up PR?
Yes. Currently, lpsolve55 is used in two places: here and in calculating isodesmic reactions. Once we replace the implementation at both locations, we will work on removing it as in another PR.
