Implement PEPS for Abelian LGT #53
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136474e5f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/vmc/drivers/custom_driver.py
Outdated
| self._sampler_key, init_key = jax.random.split(self._sampler_key) | ||
| self._sampler_configuration = jax.vmap(self.model.random_physical_configuration)( | ||
| jax.random.split(init_key, self.sampler.n_chains) |
There was a problem hiding this comment.
This new initialization path assumes self.sampler exposes n_chains and that self.model has random_physical_configuration. In current usage (e.g., DynamicsDriver built with a functools.partial of sequential_sample_with_gradients and an MPS model), neither assumption holds: the partial has no n_chains attribute and MPS defines no random_physical_configuration, so driver construction raises AttributeError before sampling starts. This is a regression for existing MPS-based drivers that previously relied on sampler-side random initialization.
Useful? React with 👍 / 👎.
Signed-off-by: 周唤海 <albus.zhouhh@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ce159367d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/vmc/drivers/custom_driver.py
Outdated
| self._sampler_key, init_key = jax.random.split(self._sampler_key) | ||
| self._sampler_configuration = jax.vmap(self.model.random_physical_configuration)( | ||
| jax.random.split(init_key, self.sampler.n_chains) |
There was a problem hiding this comment.
This new initialization path unconditionally calls self.model.random_physical_configuration, which is not implemented by several existing models (e.g., MPS in tests/test_driver_dynamics.py). That means constructing a DynamicsDriver now raises AttributeError before any sampling occurs, breaking previously working workflows. Consider only calling this when the model provides the method (or when the sampler explicitly requires an initial configuration) to avoid crashing non-PEPS runs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc226b612
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._sampler_key, init_key = jax.random.split(self._sampler_key) | ||
| self._sampler_configuration = self.model.random_physical_configuration( | ||
| init_key, n_samples=self.sampler.n_chains | ||
| ) |
There was a problem hiding this comment.
The new initialization unconditionally reads self.sampler.n_chains to size the initial configuration. If callers provide a plain function or functools.partial (as done in existing tests/examples), that object has no n_chains attribute and DynamicsDriver fails with AttributeError before any sampling occurs. Consider deriving n_chains from sampler kwargs or defaulting to 1 when the attribute is missing.
Useful? React with 👍 / 👎.
| nl = jnp.pad(h_links, ((0, 0), (1, 0)), constant_values=0) | ||
| nr = jnp.pad(h_links, ((0, 0), (0, 1)), constant_values=0) | ||
| nu = jnp.pad(v_links, ((1, 0), (0, 0)), constant_values=0) | ||
| nd = jnp.pad(v_links, ((0, 1), (0, 0)), constant_values=0) | ||
| sites = (self.Qx - nl - nd + nu + nr) % self.N |
There was a problem hiding this comment.
_single_physical_configuration computes site values modulo self.N and never clamps them to phys_dim. When phys_dim < N (e.g., a pure-gauge configuration with phys_dim=1, N=3), this can yield site indices ≥ phys_dim, which later cause out-of-bounds indexing when tensors are accessed by sites[r, c]. Either restrict configs to phys_dim >= N or wrap sites into [0, phys_dim).
Useful? React with 👍 / 👎.
No description provided.