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

Miscellaneous improvements to approx() #3741

Merged
merged 16 commits into from Aug 2, 2018

Conversation

Projects
None yet
4 participants
@kalekundert
Copy link
Contributor

commented Jul 31, 2018

This PR fixes a couple small issues with approx():

  • Change the output of repr(approx(np.array(...))) to reflect the dimension of the array being compared. For example, repr(approx([[1, 2], [3, 4]])) would look something like approx([[1±..., 2±...], [3±..., 4±...]]) rather than approx([1±..., 2±..., 3±..., 4±...]). This was supposed to fix #3712, but it turns out that #3615 already fixed it. Still, I think it's nice to have a more accurate repr.
  • Check for type errors as soon as the approx object is instantiated, rather than waiting until a comparison is made. Fixes #3473. This includes very clear error messages if the user tries to use nested dictionaries or lists, since people seem to assume that this will work. (And maybe it should, but that's a different discussion.)
  • Improve some comments.

kalekundert added some commits Jul 31, 2018

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

Hi @kalekundert, thanks a lot for the PR.

I did not take a good look at it yet, but a cursory glance reminds me that we should add changelog notes to the PR. Feel free to add more than one if you feel like it. 👍

@coveralls

This comment has been minimized.

Copy link

commented Jul 31, 2018

Coverage Status

Coverage increased (+0.1%) to 92.605% when pulling a5c0fb7 on kalekundert:approx_misc_tweaks into 2534193 on pytest-dev:master.

@@ -54,6 +54,9 @@ Bug Fixes
- `#3695 <https://github.com/pytest-dev/pytest/issues/3695>`_: Fix ``ApproxNumpy`` initialisation argument mixup, ``abs`` and ``rel`` tolerances were flipped causing strange comparsion results.
Add tests to check ``abs`` and ``rel`` tolerances for ``np.array`` and test for expecting ``nan`` with ``np.array()``

- `#3712 <https://github.com/pytest-dev/pytest/issues/3712>`_: Correctly represent the dimensions of an numpy array when calling ``repr()`` on ``approx()``.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 31, 2018

Member

Sorry, I should have been more clear: you should add changelog files to the changelog/ folder. See the README on https://github.com/pytest-dev/pytest/tree/master/changelog for more instructions. 👍

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 1, 2018

Member

Nevermind, I did some a small tweak in your branch and fixed the changelog notes myself. 👍 😁

nicoddemus added some commits Aug 1, 2018

Improve error message for invalid types passed to pytest.approx()
* Hide the internal traceback
* Use !r representation instead of !s (the default for {} formatting)
@nicoddemus
Copy link
Member

left a comment

Awesome work, thank you a lot!

for x in self.expected.values():
if isinstance(x, type(self.expected)):
raise TypeError(
"pytest.approx() does not support nested dictionaries, e.g. {!r}".format(

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

for extra support this should also include the key first and it might help to use "pretty formatting" to split larger dicts to something readable

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 1, 2018

Member

Good idea, done:

____________________________________ test1 ____________________________________

    def test1():
>       pytest.approx({'a': 1.2, 'b': {1:2}})
E       TypeError: pytest.approx() does not support nested dictionaries: key='b' value={1: 2}
E         full mapping={'a': 1.2, 'b': {1: 2}}

.tmp\foo.py:5: TypeError
____________________________________ test2 ____________________________________

    def test2():
>       pytest.approx([1,2,3, [1,2,3]])
E       TypeError: pytest.approx() does not support nested data structures: [1, 2, 3] at index 3
E         full sequence: [1, 2, 3, [1, 2, 3]]

.tmp\foo.py:8: TypeError
____________________________________ test3 ____________________________________

    def test3():
>       pytest.approx('foo')
E       TypeError: cannot make approximate comparisons to non-numeric values: 'foo'

.tmp\foo.py:11: TypeError
____________________________________ test4 ____________________________________

    def test4():
>       pytest.approx([1,2,3, ['foo']])
E       TypeError: pytest.approx() does not support nested data structures: ['foo'] at index 3
E         full sequence: [1, 2, 3, ['foo']]

.tmp\foo.py:14: TypeError
____________________________________ test5 ____________________________________

    def test5():
>       pytest.approx({'a': 1.2, 'b': {1:'foo'}})
E       TypeError: pytest.approx() does not support nested dictionaries: key='b' value={1: 'foo'}
E         full mapping={'a': 1.2, 'b': {1: 'foo'}}

.tmp\foo.py:17: TypeError
========================== 5 failed in 0.12 seconds ===========================
np_array = np.array([5.])
assert approx(np_array) == 5.0
assert repr(approx(np_array)) == string_expected
examples = [

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

can we put the parameter to np.array into a parametrize - the importorskip and wrapping can stay in the test

This comment has been minimized.

Copy link
@nicoddemus
@RonnyPfannschmidt
Copy link
Member

left a comment

as extra note i realized that is_numpy_array can be massively simplified by sys.modules.get('numpy') followed by a getattr(...., 'ndarray', None)

also a potential follow-up is support for pypys micronumpy`

# It might be nice to rewrite this function to account for the
# shape of the array...
import numpy as np
def recursive_map(f, x):

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

this helper is independent, lets move it out

This comment has been minimized.

Copy link
@nicoddemus
for key, value in self.expected.items():
if isinstance(value, type(self.expected)):
msg = "pytest.approx() does not support nested dictionaries: key={!r} value={!r}\n full mapping={}"
raise TypeError(msg.format(key, value, pprint.pformat(self.expected)))

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

lovely 👍

@@ -439,6 +443,13 @@ def test_foo():
["*At index 0 diff: 3 != 4 * {}".format(expected), "=* 1 failed in *="]
)

@pytest.mark.parametrize(
"x", [None, "string", ["string"], [[1]], {"key": "string"}, {"key": {"key": 1}}]

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

i believe test ids would help here

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 1, 2018

Member

Done 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

as extra note i realized that is_numpy_array can be massively simplified by sys.modules.get('numpy') followed by a getattr(...., 'ndarray', None)

Done as well, let me know if that's what you had in mind. 👍

@RonnyPfannschmidt
Copy link
Member

left a comment

fabulous 👍

pass


def recursive_map(f, x):

This comment has been minimized.

Copy link
@kalekundert

kalekundert Aug 1, 2018

Author Contributor

I'm a little hestiant about this being a module-level function, because it only works for nested lists (e.g. not tuples or any other kind of iterable). That's fine for ApproxNumpy.__repr__(), because numpy.tolist() is guaranteed to return nested lists, but this isn't really a generally useful function.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 1, 2018

Member

You got a point, but I think it doesn't hurt leaving it there; it is not part of the public API after all. @RonnyPfannschmidt any thoughts?

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2018

Member

name it recursive_list_map

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 1, 2018

Member

Done, renamed it _recursive_list_map as well to make it clear it is an internal API.

@kalekundert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

Thanks for all the revisions!

@nicoddemus nicoddemus merged commit 804fc40 into pytest-dev:master Aug 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kalekundert kalekundert deleted the kalekundert:approx_misc_tweaks branch Aug 2, 2018

@blueyed blueyed referenced this pull request Aug 25, 2018

Closed

Issue #3473 #3474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.