Skip to content
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

Reimplement Clar Optimization with Scipy MILP #2623

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xiaoruiDong
Copy link
Contributor

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

  1. 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.
  2. 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.

1. Replace lpsolve APIs with the scipy milp APIs. The new implementation potentially have a slightly better performance (due to vectorization, less data transfer, comparable solver performance, etc.) and improved readability.
2. 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 and not modified at all. Therefore analyzing once is enough.
@xiaoruiDong xiaoruiDong self-assigned this Mar 5, 2024
@xiaoruiDong xiaoruiDong added Module: Resonance Complexity: Medium Status: WIP This is currently work-in-progress RMG-Py Python 3.11 Transition PRs and Issues related to transitioning from Python 3.7 to 3.11 labels Mar 5, 2024
Copy link
Contributor

@hwpang hwpang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! Do we have a test for the clar optimization to test this part of code?

rmgpy/molecule/resonance.py Outdated Show resolved Hide resolved
Co-authored-by: Hao-Wei Pang <45482070+hwpang@users.noreply.github.com>
@rwest
Copy link
Member

rwest commented Mar 5, 2024

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?

@xiaoruiDong
Copy link
Contributor Author

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.

And by default during the resonance structure generation for PAHs, RMG will also run clar optimization every time.

@xiaoruiDong
Copy link
Contributor Author

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?

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.

Copy link
Contributor

@hwpang hwpang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Happy to merge it once tests pass.

@JacksonBurns
Copy link
Contributor

Blocked by #2553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Medium Module: Resonance Python 3.11 Transition PRs and Issues related to transitioning from Python 3.7 to 3.11 RMG-Py Status: WIP This is currently work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants