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

Alex/add eps #124

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Alex/add eps #124

wants to merge 3 commits into from

Conversation

atong01
Copy link
Owner

@atong01 atong01 commented Jul 11, 2024

add fixed epsilon option following #122

tests/test_conditional_flow_matcher.py Show resolved Hide resolved
@@ -115,13 +123,14 @@ def test_fm(method, sigma, shape):
x0, x1 = sample_plan(method, x0, x1, sigma)

torch.manual_seed(TEST_SEED)
if test_eps:
# compute to get same t seed
eps = torch.randn_like(x0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is this one used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay I get it. Why don't you give eps instead of ret_eps in line 132 and drop the comment? esp and ret_eps are supposed the be the same.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think we want to test that they are the same? Make sure we didn't mess anything up.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ahh no we can't do that. It messes up the seeds for later inits.

x0, x1, return_noise=True, eps=eps
)
if test_eps:
assert torch.allclose(ret_eps, eps)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this assert should be done at the end of the file. eps is supposed to be the same as ret_eps and it is in the same spirit as the tests over t_given_init

return torch.randn_like(x)

def sample_location_and_conditional_flow(self, x0, x1, t=None, return_noise=False):
def sample_location_and_conditional_flow(self, x0, x1, t=None, return_noise=False, eps=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you run some notebooks to ensure the behaviour is still correct please? thx.

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.

3 participants