Update CI python

[leejuyuu]

Update the python version in CI to fix CI failure. Fixes the test breakage due to package updates.

Fixes #117.

[kmkurn]

Thank you for the PR! It looks good overall. I've left some comments and a request before I can merge it.

[kmkurn]

Can you explain what assert_close and assert_allclose do that approx doesn't?

[leejuyuu]

The problem is not in approx itself, but it requires converting from torch tensor to python scalar, likely through numpy. I started to see RuntimeError: Can't call numpy() on Tensor that requires grad. Use tensor.detach().numpy() instead. when using torch 1.4. Instead of adding .detach().numpy().item() all over the place, it feels simpler and more readable to use dedicated testing utilities provided by torch itself.

[leejuyuu]

I'll change the commit message to make it more clear. The error mentioned in the commit seemed to only happen in my local environment.

[kmkurn]

I see. I agree that adding .detach().numpy().item() all over the place is less readable. However, I believe Pytest's approx has a nicely formatted error message when the test fails, which is also important in terms of readability. Does PyTorch's assert_close have it too? Can you please compare them and share it here?

[leejuyuu]

Yes. The following code

def test_approx():
    a = 1/3
    b = 0.3
    assert a == approx(b)

def test_torch_assert_close():
    a = torch.tensor(1/3)
    b = torch.tensor(0.3)
    torch.testing.assert_close(b, a)

gives

[kmkurn]

Thanks! The PyTorch one is readable too, which is good!

Now, to improve readability further, can you please move the calling logic to a separate function? Something like:

def assert_close(value, /, expected):
    fn_name = "assert_close" if Version(torch.__version__) >= Version("1.9.0") else "assert_allclose"
    assert_fn = getattr(torch.testing, fn_name)
    return assert_fn(value, expected, atol=1e-12, rtol=1e-6)

Then you can use assert_close in all the tests to replace approx.

Also, if approx is no longer used anywhere, please delete its import statement (line 5).

[leejuyuu]

The requested changes have been added. I did not use string literals for function names because it makes editor hovering and completions impossible.

[coveralls]

coverage: 100.0% (+100.0%) from 0.0%
when pulling 07c06d3 on leejuyuu:update-ci-python
into c885fba on kmkurn:master.

[kmkurn]

Thanks for your clarifications. I've left further requests to be addressed.

[kmkurn]

I see. I agree that adding .detach().numpy().item() all over the place is less readable. However, I believe Pytest's approx has a nicely formatted error message when the test fails, which is also important in terms of readability. Does PyTorch's assert_close have it too? Can you please compare them and share it here?

[kmkurn]

Thanks for responding to my requests. Please see my further comments below. Just a bit more change before I can merge this!

[kmkurn]

Thanks! The PyTorch one is readable too, which is good!

Now, to improve readability further, can you please move the calling logic to a separate function? Something like:

def assert_close(value, /, expected):
    fn_name = "assert_close" if Version(torch.__version__) >= Version("1.9.0") else "assert_allclose"
    assert_fn = getattr(torch.testing, fn_name)
    return assert_fn(value, expected, atol=1e-12, rtol=1e-6)

Then you can use assert_close in all the tests to replace approx.

Also, if approx is no longer used anywhere, please delete its import statement (line 5).

[kmkurn]

Thanks for addressing the requests. All good! I can merge this PR now.