Skip to content
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

Fix _call not implemented error in call_and_ladj #14

Merged
merged 6 commits into from
Feb 14, 2023

Conversation

felixdivo
Copy link
Contributor

@felixdivo felixdivo commented Feb 13, 2023

Some transformations do not implement _call, even some of the Pytorch ones like ComposeTranform.

https://github.com/pytorch/pytorch/blob/4869929f32c176dc3e5ea4cd4164b3fd73a0c9ea/torch/distributions/transforms.py#L270-L385

This results in call_and_ladj raising a NotImplementedError.

>>> import zuko
>>> t = zuko.transforms.ComposeTransform([])
>>> t.call_and_ladj(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../zuko/transforms.py", line 41, in _call_and_ladj
    y = self._call(x)
  File ".../torch/distributions/transforms.py", line 176, in _call
    raise NotImplementedError
NotImplementedError

@felixdivo felixdivo changed the title Fix error when using some torch.distributions.Transform Fix error when using some torch.distributions.Transforms Feb 13, 2023
@felixdivo felixdivo marked this pull request as ready for review February 13, 2023 13:33
@felixdivo
Copy link
Contributor Author

Curiously, the test fails if I add ComposeTransform([IdentityTransform(), SinTransform()]). I think this is unrealted, but I think should be investigated?

tests/test_transforms.py Outdated Show resolved Hide resolved
tests/test_transforms.py Outdated Show resolved Hide resolved
zuko/transforms.py Outdated Show resolved Hide resolved
@francois-rozet
Copy link
Member

Hello @felixdivo,

Thanks for reporting the bug and finding a solution. Could you address my comments before I merge?

the test fails if I add ComposeTransform([IdentityTransform(), SinTransform()])

This is because SinTranform is bijective between $-\frac{\pi}{2}$ and $\frac{\pi}{2}$. In ComposeTransform, the domain constraint is invisible for the test. So it defaults to $[-5, 5]$.

As a side note, per the contribution guidelines:

If you found a bug and want to fix it, please create an issue reporting the bug before creating a pull request. Similarly, if you want to add a new feature, first create a feature request issue. This allows to separate the discussions related to the bug/feature, from the discussions related to the fix/implementation.

@francois-rozet francois-rozet changed the title Fix error when using some torch.distributions.Transforms Fix _call not implemented error in call_and_ladj Feb 13, 2023
@francois-rozet francois-rozet merged commit 4fba662 into probabilists:master Feb 14, 2023
@felixdivo
Copy link
Contributor Author

As a side note, per the contribution guidelines

Thanks for the hint and sorry for not reading it. I'll follow it from now on. I wasn't aware of it.

@felixdivo felixdivo deleted the patch-1 branch February 15, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants