feat: add protocols for sample proposal and accept/reject#1722
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1722 +/- ##
==========================================
- Coverage 88.51% 84.71% -3.81%
==========================================
Files 137 137
Lines 11527 11518 -9
==========================================
- Hits 10203 9757 -446
- Misses 1324 1761 +437
Flags with carried forward coverage won't be shown. Click here to find out more.
|
28c58d3 to
0d50d00
Compare
dgedon
left a comment
There was a problem hiding this comment.
I don't fully get why this change is necessary and whether it is implemented in a way that has lots of code overhead for limited gain. Maybe you can clarify.
|
Thanks for the review @dgedon To clarify: the PR description refers to an earlier change (PR #1370) that converted proposal from torch.Distribution to Callable. This PR is about making that Callable type more precise with Protocols. On whether Protocols are necessary vs The proposals ( For |
dgedon
left a comment
There was a problem hiding this comment.
Thanks for the answers, it clarifies it well. Good to merge
Closes #1395
Context
In #1370, the proposal argument in
accept_reject_samplewas changed fromtorch.DistributiontoCallableto support passing sample functions directly. This made the type too permissive - there's no enforcement that the callable has the expected signature.Motivation
Replace generic
Callabletypes with PythonProtocolsto enable static type checking of the expected signatures. This catches mismatches at type-check time rather than runtime.Changes
SampleProposalprotocol:(sample_shape: torch.Size, **kwargs) -> TensorAcceptRejectFnprotocol:(theta: Tensor) -> Tensor