Skip to content
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

[Feature] update walkers overlap before computing local energy #253

Open
jiangtong1000 opened this issue Sep 18, 2023 · 4 comments · Fixed by #254
Open

[Feature] update walkers overlap before computing local energy #253

jiangtong1000 opened this issue Sep 18, 2023 · 4 comments · Fixed by #254
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@jiangtong1000
Copy link
Collaborator

def compute_estimator(self, system, walkers, hamiltonian, trial, istep=1):
trial.calc_greens_function(walkers)

Here we should have walkers.ovlp = trial.calc_greens_function(walkers) to update the ovlp too

@jiangtong1000 jiangtong1000 added the bug Something isn't working label Sep 18, 2023
@jiangtong1000 jiangtong1000 changed the title [BUG] [BUG] update walkers overlap before computing local energy Sep 18, 2023
@linusjoonho
Copy link
Collaborator

@jiangtong1000 Could you create a branch which implements the proposed fix and create a pull request?

@fdmalone
Copy link
Collaborator

fdmalone commented Sep 19, 2023

Wait, why? I'm a little worried we're assuming something about the state of the walkers overlap now (although we probably were before). This function should be renamed calculate_energy_and_update_walkers_overlap and documented. I'm sort of against having estimators modify the state of the walker, it seems dangerous. Do we recompute the overlap before propagation?

@linusjoonho
Copy link
Collaborator

@fdmalone @jiangtong1000 I actually remember this bug now. The way I fixed last time was to compute overlap consistently in the end of the propagation function. @jiangtong1000 could you not achieve what you wanted to do by evaluating overlap properly in the propagator?

@fdmalone fdmalone added documentation Improvements or additions to documentation and removed bug Something isn't working labels Sep 20, 2023
@fdmalone fdmalone changed the title [BUG] update walkers overlap before computing local energy [Feature] update walkers overlap before computing local energy Sep 20, 2023
@fdmalone
Copy link
Collaborator

I relabelled as it's not a bug but a dangerous implementation feature we should try to document as part of some developer documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants