Skip to content

Remove reimplementation of np.testing.assert_allclose #562

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaharvey8
Copy link

@jaharvey8 jaharvey8 commented Dec 19, 2023

Description

I think I got all the unittest_tools.assert_allclose instances

The following files no longer used functions from unittest_tools so I removed the import
tests/link/test_vm.py
tests/tensor/test_variable.py
tests/scalar/test_basic.py
tests/sparse/test_rewriting.py
tests/link/c/test_params_type.py
tests/tensor/rewriting/test_elemwise.py

There was a mention of utt.assert_all close in the creating_an_op document file so I changed that as well.

Related Issue

Checklist

  • Checked that the pre-commit linting/style checks pass
  • Included tests that prove the fix is effective or that the new feature works
  • Added necessary documentation (docstrings and/or example notebooks)
  • If you are a pro: each commit corresponds to a [relevant logical changes

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94
Copy link
Member

Seems like the rtol/atol values may need to be tweaked slightly, as some tests are now failing

@jaharvey8
Copy link
Author

Seems like the rtol/atol values may need to be tweaked slightly, as some tests are now failing

Potentially dumb question, is that something I need to do?

@ricardoV94
Copy link
Member

Yes, we can only merge the changes if all tests pass. You can see which tests failed here on GitHub

@jaharvey8
Copy link
Author

Oh ok yeah I just wasn't sure what the atol and rtol values were and if that was something I was supposed to modify. But I'll look and see which tests failed and why. Thanks!

@jaharvey8
Copy link
Author

Ok I fixed up some remaining calls to the old unittest_tools assert_allclose function and fixed some typos I had introduced.

On the rtol and atol issues, in the current unittest_tools assert_allclose function, the default values for those are set to 'None'. That is

def assert_allclose(expected, value, rtol=None, atol=None):
    if not _allclose(expected, value, rtol, atol):
        raise WrongValue(expected, value, rtol, atol)

Should I also pass 'None' to the np.testing.assert_allclose function calls (assuming that's allowed)?

@ricardoV94
Copy link
Member

Instead of none, we should not specify them at all (unless the test fails, then we need to tweak them)

@ricardoV94
Copy link
Member

ricardoV94 commented Jan 7, 2024

If things get out of hand, the default atol and rtol were being defined here:

def _get_atol_rtol(a, b):

And used here:

def _allclose(a, b, rtol=None, atol=None):

@Dhruvanshu-Joshi
Copy link
Member

Hi @jaharvey8 Are you still working on this PR? I'd like to take it up if you are not.

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.

Remove reimplementation of np.testing.assert_allclose
3 participants