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

Support chunking on nkjax.expect #1810

Merged
merged 6 commits into from
May 27, 2024
Merged

Conversation

PhilipVinc
Copy link
Member

@PhilipVinc PhilipVinc commented May 20, 2024

Add support for chunking to nk.jax.expect.

@PhilipVinc PhilipVinc requested a review from inailuig May 20, 2024 20:09
@PhilipVinc
Copy link
Member Author

@inailuig what do you think?

@inailuig
Copy link
Collaborator

@inailuig what do you think?

seems ok

@PhilipVinc PhilipVinc marked this pull request as ready for review May 26, 2024 20:39
@PhilipVinc PhilipVinc merged commit 3898574 into netket:master May 27, 2024
8 of 10 checks passed
@PhilipVinc PhilipVinc deleted the pv/chunk-expect branch May 27, 2024 15:38
@jwnys
Copy link
Collaborator

jwnys commented May 28, 2024

Just a small question about this implementation:

Why is f(x) computed first in the forward separately? I'm guessing it's to compute ΔL_σ for which you need the mean in the backward. Given that f(x) is typically the expensive part to compute (since it typically contains connected elements, think about the Elocs): wouldn't it be better (maybe not feasible?) to compute the vjp once for the second term (thereby computing \nabla f(x)). This should give directly also f(x) for free. Using this on all samples, you can compute ΔL_σ, and with it, compute \nabla \log p(x) ΔL_σ, which is just the derivative of the log_pdf. This would just avoid computing f(x) twice, which I think is the case now: once in forward, once in backward.

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

Successfully merging this pull request may close these issues.

None yet

3 participants