Separate iid from set x - #1945
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
- 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
…d set_x inside vi_divergence; remove set_x usage inside bind()
…ibute 'guidance_method'
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gsoc-2026 #1945 +/- ##
=============================================
- Coverage 87.99% 87.91% -0.09%
=============================================
Files 144 144
Lines 13499 13756 +257
=============================================
+ Hits 11879 12094 +215
- Misses 1620 1662 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Resolved conflicts: - likelihood_based_potential.py: keep HEAD (direct attribute set) - posterior_based_potential.py: keep HEAD (direct attribute set) - ratio_based_potential.py: keep HEAD (direct attribute set) - vector_field_potential.py: keep HEAD (direct attribute set + guidance_method) - conditional_density_utils.py: accept upstream (fixes _x_is_iid on bound)
| def set_x(self, x_o: Optional[Tensor], x_is_iid: Optional[bool] = True): | ||
| """Check the shape of the observed data and, if valid, set it.""" | ||
| if x_o is not None: | ||
| x_o = process_x(x_o).to(self.device) | ||
| self._x_o = x_o | ||
| self._x_is_iid = x_is_iid |
There was a problem hiding this comment.
Shouldn't we remove this part in this PR already?
There was a problem hiding this comment.
Ah no, we add the deprecation warning in #1946
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "LikelihoodBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| from sbi.utils.user_input_checks import process_x |
There was a problem hiding this comment.
we should import that not within the function but at file level
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "RatioBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| from sbi.utils.user_input_checks import process_x |
There was a problem hiding this comment.
same is in the likelihoood based potential
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = False) -> "PosteriorBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| from sbi.utils.user_input_checks import process_x |
There was a problem hiding this comment.
same is in the likelihoood based potential
| """ | ||
| super().set_x(x_o, x_is_iid) | ||
| if x_o is not None: | ||
| from sbi.utils.user_input_checks import process_x |
| device=self.device, | ||
| iid_method=self.iid_method, | ||
| iid_params=self.iid_params, | ||
| iid_method=iid_method or self.iid_method, |
There was a problem hiding this comment.
this should proabbly also be a id_method if iid_method is not None else self.iid_method,
| validate_args=( | ||
| prior._validate_args if validate_args is None else validate_args | ||
| ), | ||
| validate_args=False, |
There was a problem hiding this comment.
flag (see file level comment)
| def __deepcopy__(self, memo): | ||
| """Ensure prior 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.
flag (see file level comment)
There was a problem hiding this comment.
Here we seem to have the same modifications that we dicussed in #1943 before.
- there is code duplication for
__deepcopy__ - I don't see where deepcopy is used and why it is necessary to introduce
- we silently change the validate_args hard coded to False which seems to be a bug.
There was a problem hiding this comment.
ouh sorry. I messed up the merge. Will fix it.
| if x_o is not None: | ||
| x_o = process_x(x_o).to(self.device) | ||
| self._x_o = x_o | ||
| self._x_is_iid = True |
There was a problem hiding this comment.
compared with set_x() we hard to _x_is_iid to True here. Since it's not a method argument, maybe True is a good default. We have to nesure though that it is set correctly in each inherited class. Because right now by calling super() in e.g. the likelihood potential, we will ahve x_is_iid=True by default
There was a problem hiding this comment.
right now by calling super() in e.g. the likelihood potential, we will ahve x_is_iid=True by default
This is exactly what is happening right now on the main branch. likelihood calls super without _x_is_iid : https://github.com/sbi-dev/sbi/blob/main/sbi/inference/potentials/likelihood_based_potential.py#L80
BasePotential.init always calls set_x without any iid information: https://github.com/sbi-dev/sbi/blob/main/sbi/inference/potentials/base_potential.py#L33
x_is_iid is set with default value True: https://github.com/sbi-dev/sbi/blob/main/sbi/inference/potentials/base_potential.py#L60
…_method for VectorFieldBasedPotential
This branch builds upon #1943 . There I build
bind()that internally still usersset_xfor setting _x_o and x_is_iid. This PR is about resolving this dependence entirely and removing set_x from inside bind.3 things: remove set_x from BasePotential.init; replace overlooked set_x inside vi_divergence; remove set_x usage inside bind()
This PR experiments with the next step for the GSoC project: decouple iid from set_x.
It requires #1943 to be merged, which is not the case yet so, I took the commits from there with me.