-
Notifications
You must be signed in to change notification settings - Fork 247
Separate iid from set x #1945
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
Separate iid from set x #1945
Changes from 11 commits
89bc9a2
3e52f56
30459d9
8fdc10c
968b58b
84c4731
ee2adfa
0fe9359
e8444c5
81b0709
04ccd2c
caa994d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,13 +96,17 @@ def to(self, device: Union[str, torch.device]) -> "LikelihoodBasedPotential": | |
|
|
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should import that not within the function but at file level |
||
|
|
||
| bound = LikelihoodBasedPotential( | ||
| likelihood_estimator=self.likelihood_estimator, | ||
| prior=self.prior, | ||
| x_o=None, | ||
| device=self.device, | ||
| ) | ||
| bound.set_x(x_o, x_is_iid=x_is_iid) | ||
| x_o = process_x(x_o).to(self.device) | ||
| bound._x_o = x_o | ||
| bound._x_is_iid = x_is_iid | ||
| return bound | ||
|
|
||
| def __call__(self, theta: Tensor, track_gradients: bool = True) -> Tensor: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,20 +102,29 @@ def to(self, device: Union[str, torch.device]) -> "PosteriorBasedPotential": | |
|
|
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same is in the likelihoood based potential |
||
|
|
||
| bound = PosteriorBasedPotential( | ||
| posterior_estimator=self.posterior_estimator, | ||
| prior=self.prior, | ||
| x_o=None, | ||
| device=self.device, | ||
| ) | ||
| bound.set_x(x_o, x_is_iid=x_is_iid) | ||
| x_o = process_x(x_o).to(self.device) | ||
| bound._x_o = x_o | ||
| bound._x_is_iid = x_is_iid | ||
| return bound | ||
|
|
||
| def set_x(self, x_o: Optional[Tensor], x_is_iid: Optional[bool] = False): | ||
| """ | ||
| Check the shape of the observed data and, if valid, set it. | ||
| """ | ||
| super().set_x(x_o, x_is_iid=x_is_iid) | ||
| if x_o is not None: | ||
| from sbi.utils.user_input_checks import process_x | ||
|
|
||
| x_o = process_x(x_o).to(self.device) | ||
| self._x_o = x_o | ||
| self._x_is_iid = x_is_iid | ||
|
|
||
| def __call__(self, theta: Tensor, track_gradients: bool = True) -> Tensor: | ||
| r"""Returns the potential for posterior-based methods. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,13 +84,17 @@ def to(self, device: Union[str, torch.device]) -> "RatioBasedPotential": | |
|
|
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same is in the likelihoood based potential |
||
|
|
||
| bound = RatioBasedPotential( | ||
| ratio_estimator=self.ratio_estimator, | ||
| prior=self.prior, | ||
| x_o=None, | ||
| device=self.device, | ||
| ) | ||
| bound.set_x(x_o, x_is_iid=x_is_iid) | ||
| x_o = process_x(x_o).to(self.device) | ||
| bound._x_o = x_o | ||
| bound._x_is_iid = x_is_iid | ||
| return bound | ||
|
|
||
| def __call__(self, theta: Tensor, track_gradients: bool = True) -> Tensor: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,8 @@ def __init__( | |
| self.vector_field_estimator.eval() | ||
| self.iid_method = iid_method | ||
| self.iid_params = iid_params | ||
| self.guidance_method: Optional[str] = None | ||
| self.guidance_params: Optional[Dict[str, Any]] = None | ||
| self.neural_ode_backend = neural_ode_backend | ||
|
|
||
| neural_ode_kwargs = neural_ode_kwargs or {} | ||
|
|
@@ -119,7 +121,12 @@ def set_x( | |
| `IIDScoreFunction`. | ||
| ode_kwargs: Additional keyword arguments for the neural ODE. | ||
| """ | ||
| super().set_x(x_o, x_is_iid) | ||
| if x_o is not None: | ||
| from sbi.utils.user_input_checks import process_x | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import on file level |
||
|
|
||
| x_o = process_x(x_o).to(self.device) | ||
| self._x_o = x_o | ||
| self._x_is_iid = x_is_iid | ||
| self.iid_method = iid_method or self.iid_method | ||
| self.iid_params = iid_params | ||
| self.guidance_method = guidance_method | ||
|
|
@@ -140,27 +147,31 @@ def bind( | |
| **ode_kwargs, | ||
| ) -> "VectorFieldBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| from sbi.utils.user_input_checks import process_x | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
|
|
||
| bound = VectorFieldBasedPotential( | ||
| vector_field_estimator=self.vector_field_estimator, | ||
| prior=self.prior, | ||
| x_o=None, | ||
| device=self.device, | ||
| iid_method=self.iid_method, | ||
| iid_params=self.iid_params, | ||
| iid_method=iid_method or self.iid_method, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should proabbly also be a |
||
| iid_params=iid_params if iid_params is not None else self.iid_params, | ||
| neural_ode_backend=self.neural_ode_backend, | ||
| ) | ||
| bound.neural_ode.params.update(self.neural_ode.params) | ||
| # Transfer learned neural ODE parameters (e.g., from training) to the bound | ||
| # potential. This preserves the trained flow model when conditioning on new x. | ||
| bound.set_x( | ||
| x_o, | ||
| x_is_iid=x_is_iid, | ||
| iid_method=iid_method, | ||
| iid_params=iid_params, | ||
| guidance_method=guidance_method, | ||
| guidance_params=guidance_params, | ||
| **ode_kwargs, | ||
| x_o = process_x(x_o).to(self.device) | ||
| bound._x_o = x_o | ||
| bound._x_is_iid = x_is_iid | ||
| bound.guidance_method = ( | ||
| guidance_method if guidance_method is not None else self.guidance_method | ||
| ) | ||
| bound.guidance_params = ( | ||
| guidance_params if guidance_params is not None else self.guidance_params | ||
| ) | ||
| if not x_is_iid and (bound._x_o is not None): | ||
| bound.flow = bound.rebuild_flow(**ode_kwargs) | ||
| elif bound._x_o is not None: | ||
| bound.flows = bound.rebuild_flows_for_batch(**ode_kwargs) | ||
| return bound | ||
|
|
||
| def __call__( | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we seem to have the same modifications that we dicussed in #1943 before.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ouh sorry. I messed up the merge. Will fix it. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| # under the Apache License Version 2.0, see <https://www.apache.org/licenses/> | ||
|
|
||
| import warnings | ||
| from copy import deepcopy | ||
| from typing import Dict, Optional, Sequence, Union | ||
|
|
||
| import torch | ||
|
|
@@ -181,18 +182,15 @@ def __init__( | |
| event_shape=torch.Size(), | ||
| validate_args=None, | ||
| ): | ||
| self.prior = prior | ||
| self.device = None | ||
| self.return_type = return_type | ||
| super().__init__( | ||
| batch_shape=batch_shape, | ||
| event_shape=event_shape, | ||
| validate_args=( | ||
| prior._validate_args if validate_args is None else validate_args | ||
| ), | ||
| validate_args=False, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flag (see file level comment) |
||
| ) | ||
|
|
||
| self.prior = prior | ||
| self.device = None | ||
| self.return_type = return_type | ||
|
|
||
| def log_prob(self, value) -> Tensor: | ||
| return torch.as_tensor( | ||
| self.prior.log_prob(value), | ||
|
|
@@ -236,6 +234,15 @@ def to(self, device: Union[str, torch.device]) -> None: | |
| self.prior = move_distribution_to_device(self.prior, device) | ||
| self.device = device | ||
|
|
||
| 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 | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flag (see file level comment) |
||
|
|
||
| class MultipleIndependent(Distribution): | ||
| """Wrap a sequence of PyTorch distributions into a joint PyTorch distribution.""" | ||
|
|
@@ -434,6 +441,15 @@ def to(self, device: Union[str, torch.device]) -> None: | |
| self.dists[i] = move_distribution_to_device(self.dists[i], device) | ||
| self.device = device | ||
|
|
||
| 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 | ||
|
|
||
|
|
||
| def build_support( | ||
| lower_bound: Optional[Tensor] = None, upper_bound: Optional[Tensor] = None | ||
|
|
@@ -523,15 +539,13 @@ class OneDimPriorWrapper(Distribution): | |
| """ | ||
|
|
||
| def __init__(self, prior: Distribution, validate_args=None) -> None: | ||
| self.prior = prior | ||
| self.device = None | ||
| super().__init__( | ||
| batch_shape=prior.batch_shape, | ||
| event_shape=prior.event_shape, | ||
| validate_args=( | ||
| prior._validate_args if validate_args is None else validate_args | ||
| ), | ||
| validate_args=False, | ||
| ) | ||
| self.prior = prior | ||
| self.device = None | ||
|
|
||
| def to(self, device: Union[str, torch.device]) -> None: | ||
| """ | ||
|
|
@@ -546,6 +560,15 @@ def to(self, device: Union[str, torch.device]) -> None: | |
| self.prior = move_distribution_to_device(self.prior, device) | ||
| self.device = device | ||
|
|
||
| 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 | ||
|
|
||
| def sample(self, *args, **kwargs) -> Tensor: | ||
| return self.prior.sample(*args, **kwargs) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compared with
set_x()we hard to_x_is_iidto 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 callingsuper()in e.g. the likelihood potential, we will ahvex_is_iid=Trueby defaultUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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