Add .bind() method to potentials - #1943
Conversation
- Add bind() method to BasePotential (abstract) and CustomPotentialWrapper - Add bind() to LikelihoodBasedPotential, PosteriorBasedPotential, RatioBasedPotential - Add bind() to VectorFieldBasedPotential with full parameter support - Add bind() to EnsemblePotential in ensemble_posterior.py - Add bind() to ConditionedPotential in conditional_density_utils.py - Fix _base_recursor to use local variable _active_holder to avoid Python scoping issues with the _active parameter - Update posteriors to use bind() instead of set_x() for immutability - Fix PytorchReturnTypeWrapper, MultipleIndependent, OneDimPriorWrapper init order and add __deepcopy__ methods for proper deepcopy support - Update error messages to reference bind() instead of set_x() - Add bind() to test FakePotential and TractablePotential classes
|
Comming from #1940 this is what this PR does Add .bind() method to potentials and fix _base_recursor scoping
Finally we got rid of all the errors with this commit :D Now only Ensemble and VectorField remains :) |
|
|
- Add bind() method to BasePotential (abstract) and CustomPotentialWrapper - Add bind() to LikelihoodBasedPotential, PosteriorBasedPotential, RatioBasedPotential - Add bind() to VectorFieldBasedPotential with full parameter support - Add bind() to EnsemblePotential in ensemble_posterior.py - Add bind() to ConditionedPotential in conditional_density_utils.py - Fix _base_recursor to use local variable _active_holder to avoid Python scoping issues with the _active parameter - Update posteriors to use bind() instead of set_x() for immutability - Fix PytorchReturnTypeWrapper, MultipleIndependent, OneDimPriorWrapper init order and add __deepcopy__ methods for proper deepcopy support - Update error messages to reference bind() instead of set_x() - Add bind() to test FakePotential and TractablePotential classes - Replace remaining set_x calls in VectorFieldPosterior with bind
|
vector field continues to fail with this:
I've already spend quite some time yesterday debugging this in #1940 maybe I'll leave this to someone more familiar with the actual method and continue with my deprecation plan? |
|
OMG it worked! The fix was adding a single line to copy the neural ODE params when binding: bound.neural_ode.params.update(self.neural_ode.params)This ensures runtime parameter modifications (like exact, atol, rtol) persist across bind() calls, which was breaking the test's log probability evaluation. |
dgedon
left a comment
There was a problem hiding this comment.
The name of the PR with the fix _base_recursor scoping is a bit misleading since it's mainly about the bind(), right. Is the base recursor scoping orthogonal?
| init_fn = self._build_mcmc_init_fn( | ||
| self.proposal, | ||
| potential_fn=potential_bound, | ||
| transform=self.theta_transform, | ||
| init_strategy=init_strategy, # type: ignore | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
Do we understand the implications of moving that inside the loop? What is happening there and is it fine to move it inside the loop?
There was a problem hiding this comment.
Here is how I understand it:
_build_mcmc_init_fntakes apotential_fnwhich is used for weighting samples insirandresampleinit strategies- Each
xiin the batch has a different bound potential (potential_.bind(xi)). The initialization needs to evaluate the likelihood at the correctxivalue to weight/resample properly - The old code passed
self.potential_fn(unbound) to_build_mcmc_init_fn - Now each
xigets its own correctly-bound init function, otherwise this fails withbind() - There's a small overhead of rebuilding init_fn for each
xi, but this is necessary for correctness.
| Tensor: initial parameters, one for each chain | ||
| """ | ||
|
|
||
| potential_ = deepcopy(self.potential_fn) |
There was a problem hiding this comment.
Do we understand why a deepcopy was necessary here and why we can replace it it with an assignment to self?
There was a problem hiding this comment.
Jan told me this on discord:
And batched MCMC does deepcopy: look at _get_initial_params_batched in mcmc_posterior.py, first line is potential = deepcopy(self.potentialfn), and then potential.set_x(xi) inside the loop over observations.
The deepcopy is there purely so the per-xi set_x calls don't hold on to the shared self.potential_fn, it's a workaround for the statefulness.And here is the nice part: you don't need to figure out how to deepcopy inside the sampling method. in the new design the deepcopy just disappears. Instead of copy-then-mutate you create one cheap bind_observation(raw_potential, xi) per observation inside the loop.
| def __deepcopy__(self, memo): | ||
| """Ensure dists attribute is preserved during deepcopy.""" | ||
| cls = self.__class__ | ||
| result = cls.__new__(cls) | ||
| memo[id(self)] = result | ||
| for k, v in self.__dict__.items(): | ||
| setattr(result, k, deepcopy(v, memo)) | ||
| return result |
There was a problem hiding this comment.
There is code duplication nwith line 237-244
There was a problem hiding this comment.
yes, I removed it.
| self.prior = move_distribution_to_device(self.prior, device) | ||
| self.device = device | ||
|
|
||
| def __deepcopy__(self, memo): |
There was a problem hiding this comment.
unexplained why this is needed. Add a comment
There was a problem hiding this comment.
you are correct here. I removed it.
| validate_args=( | ||
| prior._validate_args if validate_args is None else validate_args | ||
| ), | ||
| validate_args=False, |
There was a problem hiding this comment.
Here bind() builds a fresh ConditionedPotential. But ConditionedPotential does not call super().__init__(), so _x_is_iid only ever gets set inside set_x(). Then, bind() builds a new ConditionedPotential straight from the constructor, so _x_is_iid is never set on it. Calling .x_is_iid on a potential returned by .bind() will raise an error
There was a problem hiding this comment.
you are right. I fixed it.
|
Thanks for the review! You are right to be confused about the deepcopy and torchutils.py changes. I think they sneaked in after I started this new PR here. Let me go over everything again and remove everything that is not needed. |
|
simplifying torchutils.py and deepcopy worked successfully. All tests pass. |
| condition=self.condition, | ||
| dims_to_sample=self.dims_to_sample, | ||
| ) | ||
| bound._x_is_iid = x_is_iid |
There was a problem hiding this comment.
This will probably be changed downstream again, once we cahnge the Potentials, right? Then it is fine to keep for now.
There was a problem hiding this comment.
What would you like to change about this exactly? Isn't this resolving your concern you had?
There was a problem hiding this comment.
Yes this is fine. I was flagging it because so far we used after binding the potnetial a .set_x() here instead. And we plan to remove those set_x() calls later on. So I was wondering if we also remove that x_is_iid setting later on.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gsoc-2026 #1943 +/- ##
=============================================
- Coverage 87.99% 87.87% -0.13%
=============================================
Files 144 144
Lines 13499 13637 +138
=============================================
+ Hits 11879 11984 +105
- Misses 1620 1653 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR attempts to do the same as #1940 but without all the rebase/merge in between.