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

Deprecate CorrLCholeskyTransform in favor of upstream CorrCholeskyTransform #3199

Merged
merged 6 commits into from
May 5, 2023

Conversation

ordabayevy
Copy link
Member

Closes #3181

@ordabayevy ordabayevy changed the title Deprecate CorrLCholeskyTransform in favor of upstreamCorrCholeskyTransform Deprecate CorrLCholeskyTransform in favor of upstream CorrCholeskyTransform Apr 22, 2023
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.

Looks good after addressing a couple comments 👍

@@ -171,7 +165,6 @@ def iterated(repeats, base_fn, *args, **kwargs):
"ConditionalRadial",
"ConditionalSpline",
"ConditionalSplineAutoregressive",
"CorrLCholeskyTransform",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the new CorrCholeskyTransform apper in this list?

Copy link
Member Author

@ordabayevy ordabayevy Apr 24, 2023

Choose a reason for hiding this comment

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

It should be added along with other PyTorch transforms below:

__all__.extend(torch_transforms)

@@ -32,7 +32,7 @@
from .block_autoregressive import BlockAutoregressive, block_autoregressive
from .cholesky import (
CholeskyTransform,
CorrLCholeskyTransform,
Copy link
Member

Choose a reason for hiding this comment

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

I'd lean towards preserving the old import so as to avoid breaking existing code, but I'll defer to your judgement if you think we should delete both. (I think of "deprecation" as meaning "it still works but we no longer recommend it", whereas removing from __init__.py would be a truly breaking change)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think it makes sense to leave the old import.


from .. import constraints


def _vector_to_l_cholesky(z):
Copy link
Member

Choose a reason for hiding this comment

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

Woo hoo less math to maintain 📉

Copy link
Member Author

@ordabayevy ordabayevy left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing @fritzo . Addressed your comments.

@@ -32,7 +32,7 @@
from .block_autoregressive import BlockAutoregressive, block_autoregressive
from .cholesky import (
CholeskyTransform,
CorrLCholeskyTransform,
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think it makes sense to leave the old import.

@@ -171,7 +165,6 @@ def iterated(repeats, base_fn, *args, **kwargs):
"ConditionalRadial",
"ConditionalSpline",
"ConditionalSplineAutoregressive",
"CorrLCholeskyTransform",
Copy link
Member Author

@ordabayevy ordabayevy Apr 24, 2023

Choose a reason for hiding this comment

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

It should be added along with other PyTorch transforms below:

__all__.extend(torch_transforms)

@ordabayevy ordabayevy requested a review from fritzo April 24, 2023 17:36
@ordabayevy
Copy link
Member Author

Hi @fritzo. Can you have another look at this PR?

@fritzo fritzo merged commit f1a5d51 into dev May 5, 2023
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.

Difference in event_shape between pyro's LKJCholesky and torch's
2 participants