Skip to content

Fix _call not implemented error in call_and_ladj#14

Merged
francois-rozet merged 6 commits intoprobabilists:masterfrom
felixdivo:patch-1
Feb 14, 2023
Merged

Fix _call not implemented error in call_and_ladj#14
francois-rozet merged 6 commits intoprobabilists:masterfrom
felixdivo:patch-1

Conversation

@felixdivo
Copy link
Copy Markdown
Contributor

@felixdivo felixdivo commented Feb 13, 2023 •
edited by francois-rozet
Loading

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
Copy Markdown
Contributor Author

felixdivo commented Feb 13, 2023

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

francois-rozet
francois-rozet requested changes Feb 13, 2023
View reviewed changes
@francois-rozet
Copy link
Copy Markdown
Member

francois-rozet commented Feb 13, 2023

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
Copy Markdown
Contributor Author

felixdivo commented Feb 15, 2023

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

Reviewers

@francois-rozet francois-rozet francois-rozet approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@felixdivo @francois-rozet