[Python 3.9] Clar SciPy Re-implementation, pythoncall Debugging, Optional RMS
#2694
+3,490
−691
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
when it is missing
also update the final run example - the point of specifying the `PYTHONPATH` was so that users _don't_ have to give the full path to `rmg.py` - the `python` executable can search it out
- clearer error messages Co-authored-by: Hao-Wei Pang <[email protected]>
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
If two surface sites overlap, or if a site-adatom bond is too long, then give up on the fancy algorithm, and just let RDkit place the X atoms wherever, without them being bonded.
Previous commit put the import at the top of the file.
Adsorbates with more than one surface site were drawn weirdly, with the second or third surface site often floating up in the air. With this change, it temporarily forms a bond between the surface sites, so the layout algorithm sees them as part of a ring, then it rotates the molecule so they are at the bottom, and then it removes the bond once the coordinates are determined. There are some special cases and exceptions for things with many surface sites. Overall, though, this should make the adsorbates look much better.
…eat/resonance_scipy
|
TODO: add a pytest mark on all of the tests which require julia and then add a new makefile target or automatic detection in the testing aparatus to allow said tests to fail when julia is not installed |
JacksonBurns
changed the title
pythoncall Debugging, Optional RMS
|
Confirmed that I can now run RMG in plain Python mode with Python 3.9, following the installation instructions as of cce1f4b |
|
The latest versions of |
|
Docs build might not need to build Julia stuff anymore |
|
|
|
Need to update graphviz version to the latest |
…t, update subprocess syntax to py3
|
After optimization to get the Clar number, brute force enumerate all structures with that Clar number. |
|
Also, we should not optimize past the most optimal clar structure. |
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)