-
Notifications
You must be signed in to change notification settings - Fork 247
bind implementation still using set_x as fallback yet #1940
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
Changes from 1 commit
9323e95
3adf008
fb06850
ad66cff
963efcf
a976fb5
02d8be8
be2eb8a
35d4dea
4e3bfb7
66bf2aa
d449cd0
8ed0076
323fdc6
c9173d0
54c2e4f
e5efa02
aca0b2f
18394fe
e773937
72125c8
0d5f16a
d4c62e2
41c0a98
f38bf44
b33e300
d0c0e38
42dce39
dbf9ca5
6817f0e
b1b11c2
58d6d38
7927d8c
2c67b91
4425525
165905e
e21c90a
8da6eb4
f10969a
8482980
27bfbac
4efce23
a08d0fc
0c53752
162a733
f70ac4a
280e722
8286c83
2966cb1
f4aaefc
b63af74
c3dccf3
d1b8106
32f164b
2a5c2ce
32c6835
62e0f3c
e6053d6
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 |
|---|---|---|
|
|
@@ -49,11 +49,17 @@ def x_is_iid(self) -> bool: | |
| return self._x_is_iid | ||
| else: | ||
| raise ValueError( | ||
| "No observed data is available. Use `potential_fn.set_x(x_o)`." | ||
| "No observed data is available. Use `potential_fn.bind(x_o)`." | ||
| ) | ||
|
|
||
| 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.""" | ||
| # import warnings | ||
| # warnings.warn( | ||
| # "set_x() is deprecated, use .bind(x) instead for stateless binding", | ||
| # FutureWarning, | ||
| # stacklevel=2, | ||
| # ) | ||
| if x_o is not None: | ||
| x_o = process_x(x_o).to(self.device) | ||
| self._x_o = x_o | ||
|
|
@@ -66,7 +72,7 @@ def x_o(self) -> Tensor: | |
| return self._x_o | ||
| else: | ||
| raise ValueError( | ||
| "No observed data is available. Use `potential_fn.set_x(x_o)`." | ||
| "No observed data is available. Use `potential_fn.bind(x_o)`." | ||
| ) | ||
|
|
||
| @x_o.setter | ||
|
|
@@ -82,6 +88,20 @@ def return_x_o(self) -> Optional[Tensor]: | |
| """ | ||
| return self._x_o | ||
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "BasePotential": | ||
| """Create new potential with x bound, without mutable state. | ||
|
|
||
| Args: | ||
| x_o: Observed data to bind. | ||
| x_is_iid: Whether x represents iid observations. | ||
|
|
||
| Returns: | ||
| New potential instance with x bound. | ||
|
|
||
| Subclasses must implement this method. | ||
| """ | ||
| raise NotImplementedError(f"{self.__class__.__name__} must implement bind()") | ||
|
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. why can we not define a general binding function?
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. What would that be?
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. Actually if it is covered in all methods that inherit from the base potential, then this is fine. There is no clear default way. |
||
|
|
||
| def to(self, device: Union[str, torch.device]) -> "BasePotential": | ||
| """Move prior and x_o to the given device. | ||
|
|
||
|
|
@@ -154,3 +174,12 @@ def __call__(self, theta, track_gradients: bool = True): | |
| """ | ||
| with torch.set_grad_enabled(track_gradients): | ||
| return self.potential_fn(theta, self.x_o) | ||
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "CustomPotentialWrapper": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| return CustomPotentialWrapper( | ||
| potential_fn=self.potential_fn, | ||
| prior=self.prior, | ||
| x_o=x_o, | ||
| device=self.device, | ||
| ) | ||
|
Comment on lines
+172
to
+179
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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,17 @@ def to(self, device: Union[str, torch.device]) -> "LikelihoodBasedPotential": | |
| self.likelihood_estimator.to(device) | ||
| return self | ||
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "LikelihoodBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| 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) | ||
| return bound | ||
|
Comment on lines
+99
to
+106
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 is a different way of building bind than in the other cases. Is taht because the
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. In the current implementation of sbi the |
||
|
|
||
| def __call__(self, theta: Tensor, track_gradients: bool = True) -> Tensor: | ||
| r"""Returns the potential $\log(p(x_o|\theta)p(\theta))$. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,15 @@ def set_x(self, x_o: Optional[Tensor], x_is_iid: Optional[bool] = False): | |
| """ | ||
| super().set_x(x_o, x_is_iid=x_is_iid) | ||
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = False) -> "PosteriorBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| return PosteriorBasedPotential( | ||
| posterior_estimator=self.posterior_estimator, | ||
| prior=self.prior, | ||
| x_o=x_o, | ||
| device=self.device, | ||
| ) | ||
|
Comment on lines
+109
to
+116
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 |
||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -82,6 +82,17 @@ def to(self, device: Union[str, torch.device]) -> "RatioBasedPotential": | |
| self.ratio_estimator.to(device) | ||
| return self | ||
|
|
||
| def bind(self, x_o: Tensor, x_is_iid: bool = True) -> "RatioBasedPotential": | ||
| """Create new potential with x bound, without mutable state.""" | ||
| 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) | ||
| return bound | ||
|
Comment on lines
+85
to
+94
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 comment as for the |
||
|
|
||
| def __call__(self, theta: Tensor, track_gradients: bool = True) -> Tensor: | ||
| r"""Returns the potential for likelihood-ratio-based methods. | ||
|
|
||
|
|
||
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.
Remove those comments