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

Sine Skewed Toridial distribution #2826

Merged
merged 42 commits into from
Jun 7, 2021

Conversation

OlaRonning
Copy link
Member

@OlaRonning OlaRonning commented Apr 27, 2021

This PR introduces the Sine Skewed Toridial distribution described by Jose Ameijeiras-Alonso and Christophe Ley.

The distribution enables skewing a base distribution on a d-dimensional torus, which is useful, for example, with dihedral angles (1-torus) of peptides as can be seen on a Ramachandran plot.

Missing prior suggestion in docstring; inference method and inferable params as suggested in #2821.

@OlaRonning OlaRonning changed the title Sine Skewed Toridial distribution [WIP] Sine Skewed Toridial distribution ~~[WIP]~~ Apr 28, 2021
@OlaRonning OlaRonning changed the title Sine Skewed Toridial distribution ~~[WIP]~~ Sine Skewed Toridial distribution Apr 28, 2021
pyro/distributions/__init__.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Show resolved Hide resolved
@OlaRonning
Copy link
Member Author

@fehiepsi, thanks for the review! 👍

pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
tests/distributions/test_distributions.py Outdated Show resolved Hide resolved
@OlaRonning OlaRonning requested a review from fehiepsi May 3, 2021 08:15
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify the input shapes for base density and skewness? It is not clear to me what's the requirements so I couldn't check if the expand method and the reshape ops in sample and log_prob are valid.

pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
@OlaRonning
Copy link
Member Author

OlaRonning commented May 3, 2021

Hi @fehiepsi,

Thanks for the review! Sorry for the input dimensions being vague. Let me clarify; the requirements are base_dist.shape() == skewness.shape skewness.shape broadcastable to base_dist.shape() and base_dist.event_shape[-1] == 2. So,

ss = SineSkewed(base_dist=BaseDist(Size([3,3,2])).to_event(1), skewness=Size([3,3,2]))
assert ss.event_shape == (2,)
assert ss.batch_shape == (3,3)

ss1 = SineSkewed(base_dist=BaseDist(Size([3,3,2])).to_event(1), skewness=Size([3,3,2])).to_event(1)
assert ss1.event_shape == (3,2)
assert ss1.batch_shape == (3,)

ss2 = SineSkewed(base_dist=BaseDist(Size([3,3,2])).to_event(2), skewness=Size([3,3,2]))
assert ss2.event_shape == (3,2)
assert ss2.batch_shape == (3,)

try:
  SineSkewed(base_dist=BaseDist(Size([3,3,2])).to_event(2), skewness=Size([3,3,2])).to_event(1)
  assert 1==0
except AssertionError:
  pass

try:
  SineSkewed(base_dist=BaseDist(Size([3,3,2])).to_event(1), skewness=Size([3,3,2])).to_event(2)  
  assert 1==0
except AssertionError:
  pass

I would like for the construction ss=SineSkewed(base_dist=BaseDist(Size([3,3,2])), skewness=Size([3,3,2])).to_event(1) also to work; however, I cannot figure out how to ensure the to_event has been called on the ss object before it's used. Edit1: typo. Edit2: changed to reflect restriction to 0<event_dim<=2.

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much cleaner than before. 👍

pyro/distributions/__init__.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
tests/distributions/test_cuda.py Outdated Show resolved Hide resolved
tests/distributions/test_cuda.py Outdated Show resolved Hide resolved
fritzo
fritzo previously approved these changes Jun 3, 2021
Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after a couple tiny nits

pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
fehiepsi
fehiepsi previously approved these changes Jun 3, 2021
pyro/distributions/sine_skewed.py Outdated Show resolved Hide resolved
psi_bound = 1 - skew_phi.abs()
skew_psi = pyro.sample('skew_psi', Uniform(-1., 1.))
skewness = torch.stack((skew_phi, psi_bound * skew_psi), dim=-1)
assert skewness.shape == (num_mix_comp, 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a constraint+transform for this (in a follow-up PR). I believe we can use signed stick-breaking transform here. This way users can define distributions over skewness (or just simply use pyro.param with correct constraint). Without that, it is a bit cumbersome for users to define correct skewness over general d-torus.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that, we can have correct constraints in the distribution definition. :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be neat; I'll add it to the backlog.

@OlaRonning OlaRonning dismissed stale reviews from fehiepsi and fritzo via 4d44aa0 June 4, 2021 08:25
fehiepsi
fehiepsi previously approved these changes Jun 7, 2021
@fehiepsi fehiepsi merged commit 0decbe5 into pyro-ppl:dev Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants