bind implementation still using set_x as fallback yet - #1940
Closed
Jocho-Smith wants to merge 58 commits into
Closed
bind implementation still using set_x as fallback yet#1940Jocho-Smith wants to merge 58 commits into
Jocho-Smith wants to merge 58 commits into
Conversation
Bumps [torch](https://github.com/pytorch/pytorch) from 2.11.0 to 2.13.0. - [Release notes](https://github.com/pytorch/pytorch/releases) - [Changelog](https://github.com/pytorch/pytorch/blob/main/RELEASE.md) - [Commits](pytorch/pytorch@v2.11.0...v2.13.0) --- updated-dependencies: - dependency-name: torch dependency-version: 2.13.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
dgedon
requested changes
Jul 21, 2026
dgedon
left a comment
dgedon
left a comment
Collaborator
There was a problem hiding this comment.
I am unsure why we should exclude MCMCPosterior, VectorFieldPotential and VectorFieldPosterior from this PR. Can you justify some more? If we have failing tests, then we have to address them before merging.
Comment on lines
+57
to
+62
| # import warnings | ||
| # warnings.warn( | ||
| # "set_x() is deprecated, use .bind(x) instead for stateless binding", | ||
| # FutureWarning, | ||
| # stacklevel=2, | ||
| # ) |
|
|
||
| Subclasses must implement this method. | ||
| """ | ||
| raise NotImplementedError(f"{self.__class__.__name__} must implement bind()") |
Collaborator
There was a problem hiding this comment.
why can we not define a general binding function?
Contributor
Author
There was a problem hiding this comment.
What would that be?
Collaborator
There was a problem hiding this comment.
Actually if it is covered in all methods that inherit from the base potential, then this is fine. There is no clear default way.
Comment on lines
+99
to
+106
| 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 |
Collaborator
There was a problem hiding this comment.
This is a different way of building bind than in the other cases. Is taht because the LikelihoodBasedPotential requires a set_x afterwards as of now? Do we plan to change that?
Contributor
Author
There was a problem hiding this comment.
In the current implementation of sbi the x_is_iid handling is done through set_x, this is why I kept it for now. After all set_x calls are replaced with bind I'll build an alternative way for the iid handling, so bind is independent of this, and set_x stays for deprecation. So the PR after this one will implement this.
Comment on lines
+85
to
+94
| 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 |
Collaborator
There was a problem hiding this comment.
Same comment as for the LikelihodBasedPotential
Comment on lines
+109
to
+116
| 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, | ||
| ) |
Collaborator
There was a problem hiding this comment.
Here x_is_iid is silently discarded.
Comment on lines
+178
to
+185
| 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, | ||
| ) |
Collaborator
There was a problem hiding this comment.
Here x_is_iid is silently discarded
…d was added in __init__
Contributor
Author
I got errors which I was not able to fix yesterday, so I discarded it for this initial post, to have the idea out there for discussion today. I'll work on the errors today. |
…ut the CI runs the tests much faster then my local setup.
…oved deepcopy (and its now unused import) b/c bind creates new objects anyway. test_batched_mcmc_sample_log_prob_with_different_x passes locally now
Contributor
Author
Not sure yet how to fix this yet. |
Contributor
Author
|
Hmm. So in this last commit attempt to fix vector_field error I tried to maintain this stateful potential, by keeping it in the defined '_get_bound_potential' function. However, this didn't fix it. I tried other things as well and I'm getting to the end of my understanding of this entire VectorFieldPotential/Posterior code base and the errors I'm seeing. |
…es after copy, so modified this in the code
Contributor
Author
|
The CI also raises another error:
My last commit fixed it for me locally However, it still fails here in the CI. How is that possible? |
Contributor
Have you rebased on the recent merge of main into the gsoc-2026 branch? |
…t attempt to fix entirely
…d was added in __init__
…ut the CI runs the tests much faster then my local setup.
…oved deepcopy (and its now unused import) b/c bind creates new objects anyway. test_batched_mcmc_sample_log_prob_with_different_x passes locally now
Bumps [pillow](https://github.com/python-pillow/Pillow) from 12.2.0 to 12.3.0. - [Release notes](https://github.com/python-pillow/Pillow/releases) - [Changelog](https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst) - [Commits](python-pillow/Pillow@12.2.0...12.3.0) --- updated-dependencies: - dependency-name: pillow dependency-version: 12.3.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Contributor
Author
|
@janfb after merging I get infinite recursion errors: is that what you fixed here #1938 ? If yes can you pls merge main into gsoc-2026 again? And why is this rebase always blowing up my PRs with 'unrelated' commits? Am I doing something wrong? Can we pls agree on a 'right way' of working simultaneously? |
This reverts commit 165905e.
…d was added in __init__
…ut the CI runs the tests much faster then my local setup.
…oved deepcopy (and its now unused import) b/c bind creates new objects anyway. test_batched_mcmc_sample_log_prob_with_different_x passes locally now
…es after copy, so modified this in the code
…t attempt to fix entirely
…ut the CI runs the tests much faster then my local setup.
This reverts commit 165905e.
Contributor
Author
|
Local tests work. I think the last main merge broke something... |
Contributor
Author
|
I opened this new PR which passes all tests (with the same code): #1943 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an intermediate step to transition from
set_xtobindfor a stateless potential function. For nowbindstill usesset_xinside, b/c iid handling has to be taken care of separately. The high level idea is described in 'Pivot Idea 1' here: #1876Some notes:
potential_fn.set_x(x_o)tobond_potential = potential_fn.bind(x_o)approach for MCMCPosterior and VectorFieldPotential and VectorFieldPosterior, so I excluded it for this PR and will continue debugging in another PR.Local tests took very long somehow, so I'm not surprised if CI complains.