ENH: Allow absolute precision in assert_almost_equal (#13357)
State: closed

closes #9457

Greetings! This is my first pull-request on an open source project, so I hope I did not miss anything at least too obvious... Thank you, in advance :smile: 

I do have a few questions:

* Should I add the `versionadded` directive in all docstrings that were changed?
* Which "whatsnew" release notes should I add this change to? v1.0.0?
* Should #9457 also be closed, as from my understanding this was also the underlying issue?
* Should I be the one ticking the checkboxes below?

This fixes the issue where:

    # Fails because it's doing (1 - .1 / .1001)
    assert_almost_equal(0.1, 0.1001, check_less_precise=True)

    # Works as intuitively expected
    assert_almost_equal(
        0.1, 0.1001,
        atol=0.01,
    )



Commit message below:
-------

This commit makes `assert_almost_equal` accept both relative and
absolute precision when comparing numbers, through two new keyword
arguments: `rtol`, and `atol`, respectively.

Under the hood, `_libs.testing.assert_almost_equal` is now calling
`math.isclose`, instead of an adaptaion of
[numpy.testing.assert_almost_equal](https://docs.scipy.org/doc/numpy-1.17.0/reference/generated/numpy.testing.assert_almost_equal.html).

- [x] closes #13357 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Labels:
name: Testing
description: pandas testing functions or related to the test suite

Comments:
Just noticed that I added the issue number on the commit title, which I guess I was not supposed to. :thinking: Can I push-force to my branch and update it?
> Just noticed that I added the issue number on the commit title, which I guess I was not supposed to. 

Not an issue at all, don't worry about it.

> Greetings! This is my first pull-request on an open source project,

Welcome!

> Which "whatsnew" release notes should I add this change to? v1.0.0?

v1.0.0, correct

> Should I be the one ticking the checkboxes below?

Yes

> Should #9457 also be closed, as from my understanding this was also the underlying issue?

I haven't looked at that yet, but assuming you're right, then yes.  also add another checkbox with a `- [x] closes #9457`
Hello @joaoleveiga! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



























There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-06-21 09:44:27 UTC
> > Should #9457 also be closed, as from my understanding this was also the underlying issue?
> 
> I haven't looked at that yet, but assuming you're right, then yes. also add another checkbox with a `- [x] closes #9457`

Looking back at this, it seems like the OP wanted a different issue solved, so I won't add the tag for that issue here.
Thanks for the suggestion @jreback ! I was afraid of touching in too many parts of the code, but I definitely agree to having a similar API to `numpy.allclose`. Let's see how deprecation works :wink: 
> I would rather:
> 
> * deprecate check_less_precise
> * add rtol and atol
> 
> similar to https://docs.scipy.org/doc/numpy/reference/generated/numpy.allclose.html with the same defaults
> 
> this will be a bit more involved, but ultimately is better than adding unfamiliar keywords

Any suggestions to make these changes without breaking current functionality?

If the `rtol` and `atol` parameters are added with the same defaults, then it will be tricky to check if the user is requesting the "old behavior" with `check_less_precise=True/False`.

Should I worry about such a breaking change or is it OK?

Example of what I was doing:

```python
def assert_almost_equal(
    left,
    right,
    check_dtype: Union[bool, str] = "equiv",
    check_less_precise: Union[bool, int] = False,
    rtol: float = 1e-5,
    atol: float = 1e-8,
    **kwargs,
):
    """(...)
    check_less_precise : bool or int, default False
        (...)
        .. deprecated:: 1.0.0
           Use `rtol` and `atol` instead to define relative, and absolute
           tolerance, respectively. Similar to :func:`numpy.allclose`.
    rtol : float, default 1e-5
        Relative tolerance.
    atol : float, default 1e-8
        Absolute tolerance.
    """
    ...
```

:thinking:

A few options I thought of:

* With `rtol`/`atol` defined as `Optional[float]` one could check if the new args are defined first, and still keep the `check_less_precise` flag working as it is now.
* Ditch the `check_less_precise` functionality while keeping the argument so that the deprecation warning is more explicit.

Any ideas?
Thanks!
you can just set the default for check_less_precise = lib.no_default (grep for it)

then if check_less_precise is passed at all you warn (and then set rtol if needed)

change all refs in the pandas codebase and should be good
After looking better at how numpy handles these sort of assertions with [numpy.isclose](https://docs.scipy.org/doc/numpy/reference/generated/numpy.isclose.html), I realized that perhaps it would be cleaner to just use it, or call [math.isclose](https://docs.python.org/dev/library/math.html#math.isclose) instead.

WDYT? Does it make sense to replicate `math.isclose` in `_libs.testing.pyx`?

This would imply deprecating the `_libs.testing.decimal_almost_equal` function.

I'm going to commit the changes I made anyway, so that you can review it.

EDIT: BTW, looking at https://github.com/numpy/numpy/issues/10161 has convinced me that going with the `math.isclose` algorithm seems like the best option. I'm just unsure on how to deal with the deprecation, to avoid being a nuisance to people that might be using the `testing.assert_*_equal` functions.
> After looking better at how numpy handles these sort of assertions with [numpy.isclose](https://docs.scipy.org/doc/numpy/reference/generated/numpy.isclose.html), I realized that perhaps it would be cleaner to just use it, or call [math.isclose](https://docs.python.org/dev/library/math.html#math.isclose) instead.
> 
> WDYT? Does it make sense to replicate `math.isclose` in `_libs.testing.pyx`?
> 
> This would imply deprecating the `_libs.testing.decimal_almost_equal` function.
> 
> I'm going to commit the changes I made anyway, so that you can review it.
> 
> EDIT: BTW, looking at [numpy/numpy#10161](https://github.com/numpy/numpy/issues/10161) has convinced me that going with the `math.isclose` algorithm seems like the best option. I'm just unsure on how to deal with the deprecation, to avoid being a nuisance to people that might be using the `testing.assert_*_equal` functions.

yes not averse to using ``math.isclose`` here. I think we handle all of the NaN / different dtype stuff before then (that is of course why this was written original; numpy didn't handle anything except float/int properly), but that's where these tol's matter anyhow
Should I move the changes in the *whatsnew* file from the Enhancements section to the Deprecation section?
Meanwhile, I found some tests that use `assert_almost_equal` are breaking (and were already breaking prior to this change...). Should I try fixing them in this issue, or open a new one?
@jreback what do you think?
> @joaoleveiga can you fix up merge conflicts? Someone can take a look after that

Conflicts solved.
Looking at the checks that failed, I was left with a few questions:

1. Should I add a new test under `tests.util.test_assert_almost_equal` to test that the `FutureWarning` is raised?
2. Should the warning issue a `FutureWarning` or a `DeprecationWarning`?
3. Should I add the `@pytest.mark.filterwarnings("ignore:The 'check_less_precise':FutureWarning")` decoration to all tests that make use of `check_less_precise=`? :grimacing: 

Thanks
Do we have internal use cases for this, or are we just/mostly providing it for downstream authors/users?
> Do we have internal use cases for this, or are we just/mostly providing it for downstream authors/users?

Was this question for me? :sweat_smile: , I had some test cases in a project that were failing and upon reading the docs of `assert_almost_equal` I was really confused about why were the tests failing, and then realized the docstrings were very misleading. One thing led to another and I felt this was good excuse to contribute to pandas :eyes: 
@jbrockmendel , @jreback any update? 
@joaoleveiga sorry for the delay, can you rebase (hopefully the CI failures are unrelated) and ill take a lookl
> @joaoleveiga sorry for the delay, can you rebase (hopefully the CI failures are unrelated) and ill take a lookl

Done!
@joaoleveiga looks like a few merge conflicts have built up - can you resolve them?
> @joaoleveiga looks like a few merge conflicts have built up - can you resolve them?

Done! Sorry for the delay, but these past weeks were a bit intense here (Portugal).
this was pretty close last time i looked. can you merge master
> this was pretty close last time i looked. can you merge master

@jreback , just merged master. Locally I'm getting some errors on the datetime tests which seem to be somewhat related to locale definitions. Other than that, other tests passed :ok_hand: 
@joaoleveiga possible typing issue: https://github.com/pandas-dev/pandas/pull/30562/checks?check_run_id=772917451#step:11:10, can you check?

```
Performing static analysis using mypy
pandas/_testing.py:1183: error: "ExtensionArray" has no attribute "asi8"
```
> @joaoleveiga possible typing issue: https://github.com/pandas-dev/pandas/pull/30562/checks?check_run_id=772917451#step:11:10, can you check?
> 
> ```
> Performing static analysis using mypy
> pandas/_testing.py:1183: error: "ExtensionArray" has no attribute "asi8"
> ```

I have looked at that and I still don't understand why it's triggering. I didn't change that piece of code. 
I can add an additional `hasattr(right, "asi8")` if it's OK for you, even though it's not really related to the original issue/PR.

WDYT? @TomAugspurger , @jreback 

```python
    if hasattr(left, "asi8") and hasattr(right, "asi8") and type(right) == type(left):
```
> > @joaoleveiga possible typing issue: https://github.com/pandas-dev/pandas/pull/30562/checks?check_run_id=772917451#step:11:10, can you check?
> > ```
> > Performing static analysis using mypy
> > pandas/_testing.py:1183: error: "ExtensionArray" has no attribute "asi8"
> > ```
> 
> I have looked at that and I still don't understand why it's triggering. I didn't change that piece of code.
> I can add an additional `hasattr(right, "asi8")` if it's OK for you, even though it's not really related to the original issue/PR.
> 
> WDYT? @TomAugspurger , @jreback
> 
> ```python
>     if hasattr(left, "asi8") and hasattr(right, "asi8") and type(right) == type(left):
> ```

can you repro locally? I think something like
```assert isinstance(left, DatetimeLikeArrayMixin)``` (and same for right) will work here
@joaoleveiga want to make a PR to enforce this deprecation?
> @joaoleveiga want to make a PR to enforce this deprecation?

@jbrockmendel , I only had time to tackle this today but I see it was already taken care of :) thanks for the heads up. Cheers