-
Notifications
You must be signed in to change notification settings - Fork 373
Implement unittests for MemmapTensor. #231
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
Conversation
Signed-off-by: Fred Feng <fung@fb.com>
Signed-off-by: Fred Feng <fung@fb.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work, thanks so much for this! Almost good to go!
Signed-off-by: Fred Feng <fung@fb.com>
Signed-off-by: Fred Feng <fung@fb.com>
test/test_memmap.py
Outdated
""" | ||
value.to(device) | ||
expected_memmap_tensor = MemmapTensor(value) | ||
m1 = MemmapTensor([3, 4]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guss this guy should be created on device too
m1 = MemmapTensor([3, 4], device=device)
test/test_memmap.py
Outdated
assert m1.shape == (3, 4) | ||
assert torch.all(m1 == expected_memmap_tensor) | ||
assert torch.all(m1 + torch.ones([3, 4], device=device) == 1) | ||
m2 = MemmapTensor(3, 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
by the way, we could perhaps nest those two tests using parameters
@pytest.mark.parametrize("shape", [ [[3, 4]], [3, 4] ])
def test_foo(shape, device):
m1 = MemmapTensor(*shape, device=device) # first cast is a list, second case is a sequence of integers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vmoens!
I have made corresponding changes. I was setting up the WSL environment locally on a windows machine to test cuda/cpu. Sorry for delayed responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splendid! Thanks so much for this!
This PR closes #221.