-
Notifications
You must be signed in to change notification settings - Fork 27
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
FT cleanup #296
FT cleanup #296
Conversation
clean up thermal, add some files needed from legacy, will remove later clean up thermal
…s/uhf_walkers.py. Removed thermal/hamiltonians/ and thermal/systems/directories. Added ueg to 0T hamiltonians/ and systems/ directories.
clean up walker handler, remove qmc.calc file..
complex integrals.
`hamiltonian` in thermal/estimators/ files.
`hamiltonian` in thermal/trial/ files.
…into ft_cleanup
ipie/addons/thermal/utils/ueg.py
Outdated
import scipy.sparse | ||
from ipie.utils.io import write_qmcpack_sparse | ||
|
||
# ----------------------------------------------------------------------------- |
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.
just class UEG:
is fine. The # ---- docstrings don't add much here. Why is this in utils?
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.
The ueg.py
was only meant for testing. I think the idea was to add a UEG example for users to refer to. I can add that.
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.
Right, if it's just an example I would put it in examples (not utils). If it's a bone-fide system it should go in Hamiltonian.
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.
It's called in utils/testing.py
though--do we want it in the non-thermal hamiltonian
? I thought it was discussed to only have generic*.py
there at one point
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.
I think if we just want it for testing the more sensible (larger refactor would be), make a folder called testing, move all the testing methods into their own separate modules in testing/* and make a testing/hamiltonians.py to add new test only hamiltonians. Utils should disappear largely speaking #242. For now I guess this is ok.
Scale factor (2pi/L). | ||
""" | ||
|
||
def __init__(self, options, verbose=False): |
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.
Again not your problem but we shouldn't pass in big dicts of parameters. Feel free to not address this now.
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 for this! I know this must have been very painful.
The test is failing because you need to build the cython modules. I'm uneasy that this cythonizing is again becoming necessary. Can you include an import guard so that the tests run without the need for cythonizing (you should see in the test output that several tests are already skipped due to missing cythonized modules. |
Thanks for the feedback and looking over it all. I've tried to address all your comments--please let me know if there's anything else I should look at! |
* Allow python > 3.11 * Allow numpy >= 1.26 * Bump CI python versions (min version is 3.10) * Fix lint / format errors introduced in #296 I split the AFQMC driver into a base class and then implementations for FT / ZT afqmc which allows for more customization of the factory methods.
Cleaned up legacy thermal AFQMC code to mirror the structure of 0T code.