-
Notifications
You must be signed in to change notification settings - Fork 116
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
QCQMC Part 5: Add Trial Wavefunction #347
Conversation
7c82c04
to
06435f4
Compare
@mpharrigan / @dstrain115 PTAL |
recirq/qcqmc/trial_wf.py
Outdated
n_optimization_restarts: int = 1, | ||
do_print: bool = True, | ||
use_fast_gradients: bool = False, | ||
) -> Tuple[np.ndarray, np.ndarray, np.ndarray]: |
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 also a long and complicated function with no docstring or comments.
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.
Split it up and added a reference to the paper.
recirq/qcqmc/trial_wf.py
Outdated
print("Two Body Rotation Parameters:") | ||
print(two_body_params) | ||
|
||
return one_body_params, two_body_params, one_body_basis_change_mat |
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 agree. I think this should be split up.
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.
Split everything into modules
What were you thinking re: splitting it up?
would be one way |
Pretty much this yes:
|
optimization might be split too if possible |
One thing the refactor revealed is a lack of unit tests for quite a few things... |
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.
Like you mentioned, it looks like there are a lot of missing tests. Maybe we could add them in a later PR. Otherwise, this seems like much better organization.
recirq/qcqmc/optimize_wf_test.py
Outdated
assert trial_wf.ansatz_energy < -1.947 | ||
|
||
|
||
# TODO: Speed up this test and add a similar one with non-trivial heuristic layers. |
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.
Maybe use @pytest.mark.slow
on the relevant tests?
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.
@wjhuggins, do you remember where this is from? Running these tests only takes 7 seconds total which isn't necessarily slow, but I do see quite significant creep in the CI's duration (from 5m to 13! minutes) since these PRs started to go in. I'll follow up with a PR to make things faster.
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.
yeah somehow these are exceptionally slow for actions, but only take a few seconds on my laptop. I'll investigate
@dstrain115 could we merge and I'll follow up to add some more unit tests and fix the CI slowness. |
Yeah, that's fine with me. |
(I can't merge this myself) |
Add trial wavefunction module.
This is very big and could be split up into several files. WDYT @mpharrigan