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

[Python 3.9] Reimplement Clar Optimization with Scipy MILP #2694

Draft
wants to merge 48 commits into
base: feat/py39
Choose a base branch
from

Conversation

JacksonBurns
Copy link
Contributor

This PR replaces #2623 as discussed in this comment: #2687 (comment)

xiaoruiDong and others added 5 commits July 24, 2024 14:11
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.
Co-authored-by: Hao-Wei Pang <45482070+hwpang@users.noreply.github.com>
@JacksonBurns JacksonBurns changed the title Reimplement Clar Optimization with Scipy MILP - Python 3.9 [Python 3.9] Reimplement Clar Optimization with Scipy MILP Jul 24, 2024
@xiaoruiDong xiaoruiDong self-assigned this Jul 25, 2024
@xiaoruiDong
Copy link
Contributor

@xiaoruiDong Mark this PR.

@JacksonBurns
Copy link
Contributor Author

@xiaoruiDong here are the remaining test failures which we should address in this PR.

FAILED test/rmgpy/data/solvationTest.py::TestSoluteDatabase::test_saturation_density - AssertionError: assert 1.93 == 0
 +  where 1.93 = round(1.9298279226322848, 2)
 +    where 1.9298279226322848 = abs((6385.1498279226325 - 6383.22))
 +      where 6385.1498279226325 = get_liquid_saturation_density('Hexane', 400)
FAILED test/rmgpy/data/thermoTest.py::TestCyclicThermo::test_add_poly_ring_correction_thermo_data_from_heuristic_using_pyrene - AssertionError: assert 'Benzene' in []
FAILED test/rmgpy/data/thermoTest.py::TestMolecularManipulationInvolvedInThermoEstimation::test_bicyclic_decomposition_for_polyring_using_pyrene - assert [0, 0, 0, 0, 0] == [0, 6, 6, 6, 6]
  At index 1 diff: 0 != 6
  Full diff:
  - [0, 6, 6, 6, 6]
  + [0, 0, 0, 0, 0]
FAILED test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_f_rules - ValueError: setting an array element with a sequence. The requested array has an inhomogeneous shape after 2 dimensions. The detected shape was (7, 6) + inhomogeneous part.
FAILED test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_d_regularization_dims - Exception: Unable to calculate free energy for species 'C6H6-5': no thermo or statmech data available.
FAILED test/rmgpy/molecule/moleculeTest.py::TestMolecule::test_count_aromatic_rings - assert [2, 0] == [2, 1, 0]
  At index 1 diff: 0 != 1
  Right contains one more item: 0
  Full diff:
  - [2, 1, 0]
  ?     ---
  + [2, 0]
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_naphthyl - assert 2 == 4
 +  where 2 = len([Molecule(smiles="[c]1cccc2ccccc12"), Molecule(smiles="[C]1=C2C=CC=CC2=CC=C1")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_napthalene - assert 2 == 4
 +  where 2 = len([Molecule(smiles="Cc1cccc2ccccc12"), Molecule(smiles="CC1=CC=CC2=CC=CC=C12")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_phenanthrene - assert 2 == 3
 +  where 2 = len([Molecule(smiles="Cc1cccc2c1ccc1ccccc12"), Molecule(smiles="CC1=CC=CC2=C1C=CC1=CC=CC=C12")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_phenanthrene_radical - assert 2 == 9
 +  where 2 = len([Molecule(smiles="[CH2]c1cccc2c1ccc1ccccc12"), Molecule(smiles="[CH2]C1=CC=CC2=C1C=CC1=CC=CC=C12")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_c13h11_rad - assert 2 == 6
 +  where 2 = len([Molecule(smiles="Cc1ccc([CH]c2ccccc2)cc1"), Molecule(smiles="CC1=CC=C([CH]C2=CC=CC=C2)C=C1")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_aryne_3_rings - assert 4 == 5
 +  where 4 = len([Molecule(smiles="C1#Cc2c(ccc3ccccc23)C=C1"), Molecule(smiles="C1=C=C2C(=CC=1)C=Cc1ccccc12"), Molecule(smiles="C1#CC2=C(C=C1)C=Cc1ccccc12"), Molecule(smiles="C1#CC2=C(C=C1)C=CC1=C2C=CC=C1")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_polycyclic_aromatic_with_non_aromatic_ring2 - assert 2 == 4
 +  where 2 = len([Molecule(smiles="C=C1C=Cc2cc3c(ccc4cc5ccccc5cc43)cc2C1=C"), Molecule(smiles="C=C1C=CC2=CC3=C(C=CC4=CC5=CC=CC=C5C=C43)C=C2C1=C")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_false_negative_polycyclic_aromaticity_perception - assert 2 == 6
 +  where 2 = len([Molecule(smiles="[CH2]c1cccc2ccccc12"), Molecule(smiles="C=C1C=CC=C2C=C[CH]C=C12")])
FAILED test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_false_negative_polycylic_aromaticity_perception2 - assert 6 == 7
 +  where 6 = len([Molecule(smiles="[CH2]C=C1C=CC(=C)c2ccccc21"), Molecule(smiles="[CH2]c1ccc(C=C)c2ccccc12"), Molecule(smiles="C=C[C]1C=CC(=C)c2ccccc21"), Molecule(smiles="C=CC1=C[CH]C(=C)c2ccccc21"), Molecule(smiles="[CH2]C1=CC=C(C=C)c2ccccc21"), Molecule(smiles="[CH2]C=C1C=CC(=C)C2=C1C=CC=C2")])
FAILED test/rmgpy/molecule/resonanceTest.py::ClarTest::test_clar_optimization - ValueError: not enough values to unpack (expected 4, got 2)
FAILED test/rmgpy/molecule/resonanceTest.py::ClarTest::test_phenanthrene - assert 0 == 1
 +  where 0 = len([])
FAILED test/rmgpy/molecule/resonanceTest.py::ClarTest::test_phenalene - assert 0 == 2
 +  where 0 = len([])
FAILED test/rmgpy/molecule/resonanceTest.py::ClarTest::test_corannulene - assert 0 == 5
 +  where 0 = len([])
FAILED test/rmgpy/statmech/ndTorsionsTest.py::TestHinderedRotorClassicalND::test_hindered_rotor_nd - assert 4e-05 == 0
 +  where 4e-05 = round((2.8993250143510827 - 2.899287634962152), 5)
 +    where 2.8993250143510827 = abs(2.8993250143510827)
 +      where 2.8993250143510827 = <bound method HinderedRotorClassicalND.calc_partition_function of Mode(quantum=False)>(300.0)
 +        where <bound method HinderedRotorClassicalND.calc_partition_function of Mode(quantum=False)> = Mode(quantum=False).calc_partition_function

There are a couple test failures which look like actual failures to run (test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_f_rules and test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_d_regularization_dims) - the remained run, but produce 'incorrect' results. Some of these might not actually be incorrect, and the tests need updating. Others are likely issues with the reimplemented CLAR optimization. Please let me know how I can help get these running again!

@JacksonBurns
Copy link
Contributor Author

Hi @xiaoruiDong will you be able to work on this PR, or should we re-assign?

@xiaoruiDong
Copy link
Contributor

Hi @xiaoruiDong will you be able to work on this PR, or should we re-assign?

I will work on this but don't have time in a week or two... If this is something urgent, I am ok with re-assignment and can talk to the people who will work on this.

@JacksonBurns
Copy link
Contributor Author

It's not especially urgent - I am going to try and fix some of the failures, but we can continue to chat about it once you have the time!

@JacksonBurns
Copy link
Contributor Author

These are the commits that are specific to the CLAR re-implementation in this PR: https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2694/files/756439fd5096859f3e67002571c0a9e1c19258ff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants