-
Notifications
You must be signed in to change notification settings - Fork 48
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
Implement Spinner optimizer with docs and tests. #356
Implement Spinner optimizer with docs and tests. #356
Conversation
This pull request introduces 1 alert when merging 34e9ce4 into ca4d260 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging bfdef8a into 0071184 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing work, thank you so much @andrewtarzia. Just some very minor nits.
src/stk/molecular/topology_graphs/topology_graph/optimizers/spinner.py
Outdated
Show resolved
Hide resolved
src/stk/molecular/topology_graphs/topology_graph/optimizers/spinner.py
Outdated
Show resolved
Hide resolved
src/stk/molecular/topology_graphs/topology_graph/optimizers/spinner.py
Outdated
Show resolved
Hide resolved
src/stk/molecular/topology_graphs/topology_graph/optimizers/spinner.py
Outdated
Show resolved
Hide resolved
There are two warnings in the docs compilation, which I think were introduced by #352. Do you mind fixing them in this PR? |
Could you send the info? - very happy to make the changes. |
At the bottom of the github page you will have a "All checks have passed" row, which has a "Show all checks button". If you click on that and scroll through the different checks, there should be one for readthedocs. If you click on it, it will take you to the docs. If you lick the "v:356" in the bottom left corner and then on "builds". You can go through the builds until you find "356". there you will see the different steps. click on the last one and scroll of the way down. You will see a warning (there's only one tbh, not two, I think you must have fixed the previous ones already). You have to scroll up a bit to find the actual warning. The warning itself is because you didn't run remake-modules. WHich is fine cos thats autodamted now. So you dont actually have to do anything. But I figured I would post this for future reference. |
Ahhh amazing- I always forget about that. Will do other changes Wed. |
This pull request introduces 1 alert when merging 2c5bb7b into f1ebeda - view on LGTM.com new alerts:
|
@andrewtarzia actually you probably should run remake-modules, becaues without it you cannot see the compiled docs for Spinner, so its hard to make sure everything is OK. |
This pull request introduces 1 alert when merging a1ea33d into f1ebeda - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 3c155bd into 45170de - view on LGTM.com new alerts:
|
Requested Reviewers: @lukasturcani
Note for Reviewers: If you accept the review request add a 👍 to this post
Simply add another optimizer called Spinner, which is made to optimize/generate conformers of host-guest pairs. This is most crucial for systems with multiple guests that may have overlapping structures.
I followed the same approach as the mchammer optimizers (effectively the same interface and code).
Adds a new dependency. It is small, but worth making the decision about its inclusion.
minimum working example (tests also included) - compare the two output structures.: