[Python 3.9] Reimplement Clar Optimization with Scipy MILP #2694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added per sevy's review Co-authored-by: Sevy Harris <[email protected]>
fixed tabs
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 <[email protected]>
This was referenced Jul 24, 2024
…tion_test Deprecate the nomkl mutex metapackage
Added build arguments for Dockerfile
…orge Changed Dockerfile to use Miniforge3
|
Command to run the tests related to this PR: python-jl -m pytest \
test/rmgpy/data/solvationTest.py::TestSoluteDatabase::test_saturation_density \
test/rmgpy/data/thermoTest.py::TestCyclicThermo::test_add_poly_ring_correction_thermo_data_from_heuristic_using_pyrene \
test/rmgpy/data/thermoTest.py::TestMolecularManipulationInvolvedInThermoEstimation::test_bicyclic_decomposition_for_polyring_using_pyrene \
test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_f_rules \
test/rmgpy/data/kinetics/familyTest.py::TestTreeGeneration::test_d_regularization_dims \
test/rmgpy/molecule/moleculeTest.py::TestMolecule::test_count_aromatic_rings \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_naphthyl \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_napthalene \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_phenanthrene \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_methyl_phenanthrene_radical \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_c13h11_rad \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_aryne_3_rings \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_polycyclic_aromatic_with_non_aromatic_ring2 \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_false_negative_polycyclic_aromaticity_perception \
test/rmgpy/molecule/resonanceTest.py::ResonanceTest::test_false_negative_polycylic_aromaticity_perception2 \
test/rmgpy/molecule/resonanceTest.py::ClarTest::test_clar_optimization \
test/rmgpy/molecule/resonanceTest.py::ClarTest::test_phenanthrene \
test/rmgpy/molecule/resonanceTest.py::ClarTest::test_phenalene \
test/rmgpy/molecule/resonanceTest.py::ClarTest::test_corannulene \
test/rmgpy/statmech/ndTorsionsTest.py::TestHinderedRotorClassicalND::test_hindered_rotor_nd |
|
Notes about install:
|
|
Didn't work |
../../../miniconda3/envs/rmg_env/lib/python3.9/site-packages/rdkit/Chem/__init__.py:16: in <module>
from rdkit.Chem import rdchem
E ImportError: /home/runner/miniconda3/envs/rmg_env/lib/python3.9/site-packages/scipy/sparse/../../../../libstdc++.so.6: version `GLIBCXX_3.4.31' not found (required by /home/runner/miniconda3/envs/rmg_env/lib/python3.9/site-packages/rdkit/Chem/../../../../libboost_serialization.so.1.86.0)
|
May need to downgrade julia back to 1.8 or something like that: #2353 (comment) |
|
@rwest @mjohnson541 I am trying to get the Python 3.9 + during test startup. Any ideas? |
|
It looks like this issue here: https://discourse.julialang.org/t/glibcxx-version-not-found/82209/7 There's a fix that looks promising a few responses down...they write it in terms of CondaPkg.jl, but if we constrain libstdcxx-ng properly in the environment file it should do the same thing. |
|
Great find, thanks! We actually don't have |
|
Hmm...possibilities that occur to me:
|
|
Those both seem like good options, will consider once this gets closer to merging. Latest errors are the same as before (without attempting to limit the library version). I am going to roll in the changes from #2631 and try to get this running without Julia to start with, since I think there is a possible library incompatibility problem with RDKit and SciPy. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces #2623 as discussed in this comment: #2687 (comment)