-
Notifications
You must be signed in to change notification settings - Fork 15
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
Merge CN-SGMC into main #271
Conversation
# Conflicts: # smol/moca/__init__.py
# Conflicts: # smol/moca/__init__.py
…PeriodicNeighbor unpack
…tests to charge neutral ensemble.
# Conflicts: # requirements.txt
# Conflicts: # README.md # smol/moca/__init__.py # smol/moca/ensemble/semigrand.py
# Conflicts: # requirements.txt
for more information, see https://pre-commit.ci
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.
Thanks a lot @qchempku2017 for implementing and keeping this PR up to date! It should be great functionality.
It just needs a bit of style cleanup in using function names and variables are more obvious what they represent, to have more readable code.
It also seems to me there is a lot of code that we do not need, either because it is implemented already in numpy, math, or any other of the required packages for smol. We should minimize re-writing functions that we can directly use from well-established packages, bc this makes the code simpler, easier to maintain, and less likely to introduce bugs...
I'm also getting a failure in the unit-tests from test_solve_diop_rand
I think after addressing these then we should be ready to merge.
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.
Also it would be very helpful to have a basic example of how to use this functionality in an example jupyter notebook.
I have loosen the test criteria to the following: |
That sounds good. As long as the test still works to make sure that the code is running as intended |
Tests are confirmed to pass. |
Thanks a lot for this PR @qchempku2017 ! |
Summary
Additional dependencies introduced (if any)
sympy; (used to treat integers)
polytope (an opensource pypi package used to solve vertices of a convex polytope)
cvxpy and cvxopt (used to find the center of a polytope, and to optimize table-exchanges)
TODO (if any)
Checklist
Run pycodestyle and flake8
on your local machine.
Run pydocstyle on your code.