-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Replaced the usage of custom function assert_equal with np.testing.assert_equal #15839
base: main
Are you sure you want to change the base?
Replaced the usage of custom function assert_equal with np.testing.assert_equal #15839
Conversation
… with np.testing.assert_equal Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
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.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
👋 Thank you for your draft pull request! Do you know that you can use |
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
…with np.testing.assert_equal Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
…t_equal with np.testing.assert_allclose Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Hello @dhomeier, I completed the change mentioned in the issue. But, the following are the errors received in the Git workflow can you please help me with those?
Apart from this, all the tests ran successfully except 3 failed in my local I have attached a screenshot for those. This failure where in the docs file. |
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 for you contribution !
Just a general comment on style: in astropy, numpy testing function are usually imported one of the following ways
from numpy.testing import assert_array_equal
import numpy.testing as npt
astropy/io/ascii/tests/common.py
Outdated
|
||
|
||
# Compatibility functions to convert from nose to pytest |
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.
# Compatibility functions to convert from nose to pytest |
Comment just related to the removed functions, so this can all be deleted.
I think the idea was rather to replace those with a simple assert not isinstance(t2[name][i], str) and np.isnan(t2[name][i] According to the docs |
In addition, using the former for As for Last but not least @Rohit-Pujari has already begun work on this same task in #15787 – I don't know if there is still a perspective for completing that PR after college classes have resumed. |
The actual detailed errors must be further up in the full test log, but since doc build and tests are passing in the CI, I would not worry too much about that. |
Gotta do something about that pre-commit rule. |
You mean remove the rule? 🤔 But there were some real fixes of unused or misplaced imports that should be addressed before pushing upstream, @Harsh-pandya . Though I have no idea how the imports would have become unsorted by just removing one – perhaps you need to remove the following blank line as well? |
Not sure how to properly ignore the rule but I think it should be done locally as needed only. Maybe @eerovaher or @nstarman can advise. Thanks! |
It's usually best to use the Test for lists of numbers: import numpy as np
values = [1, 2, 3]
def test_example_1():
assert [1, 3, 3] == values
def test_example_2():
assert np.array_equal([1, 3, 3], values)
def test_example_3():
np.testing.assert_array_equal([1, 3, 3], values)
def test_example_4():
np.testing.assert_([1, 3, 3] == values) Traceback:
Test for strings: import numpy as np
message = "Expected string output."
def test_example_1():
assert "Expectd String output." == message
def test_example_2():
np.testing.assert_array_equal("Expectd String output.", message)
def test_example_3():
np.testing.assert_("Expectd String output." == message) Traceback:
|
…rts for numpy testing package. Also removed the unused import in common.py file. Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Thanks; I don't see much benefit in |
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.
As discussed I think using just assert_allclose
and assert_equal
from np.testing
will be enough.
Not sure exactly how to make Ruff happy about the import order; perhaps it will be best to let it sort this out itself in the pre-commit hook. So if possible, apply the pre-commit fixes locally on your next commit before pushing. Thanks!
import numpy as np | ||
import numpy.testing as npt | ||
import pytest | ||
from contextlib import nullcontext | ||
from io import BytesIO | ||
from textwrap import dedent |
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.
import numpy as np | |
import numpy.testing as npt | |
import pytest | |
from contextlib import nullcontext | |
from io import BytesIO | |
from textwrap import dedent | |
from contextlib import nullcontext | |
from io import BytesIO | |
from textwrap import dedent | |
import numpy as np | |
import pytest | |
from numpy.testing import assert_allclose, assert_equal |
Frankly I have no idea what import order Ruff wishes to enforce, but the previous form was accepted, so apparently alphabetical order within a block is fine...
I personally prefer the option of using assert_*
directly; since the old custom functions will be removed entirely, I don't think there is much risk of confusion. But that is up for discussion.
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.
ruff is configured to the standard isort
settings: first sort by package origin (standard library, third party, first party, local) then alphabetically within those blocks with import X
preceding from X import
.
npt.assert_( | ||
not isinstance(t2[name][i], str) and np.isnan(t2[name][i]) | ||
) |
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.
npt.assert_( | |
not isinstance(t2[name][i], str) and np.isnan(t2[name][i]) | |
) | |
assert not isinstance(t2[name][i], str) and np.isnan(t2[name][i]) |
See discussion; I don't think we want to use npt.assert_
at all after that.
No problem, as we would rather go with the alternative approach (using the |
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, got everything to pass after all!
I am leaving this open for @taldcroft and @hamogu to comment if they have any preferences on assert_equal
vs. npt.assert_equal
; for reference the existing code has 109 uses from numpy.testing import assert_something
vs. 1 of
import numpy.testing as npt
.
Will also need squashing before this can be merged!
In most if not all cases we want to replace the old |
I understood your comparison in #15839 (comment)
to exactly the opposite effect –
|
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've commented two uses of np.testing.assert_equal()
, but analogous comments could be made throughout.
@@ -42,24 +41,24 @@ def assert_table_equal(t1, t2, check_meta=False, rtol=1.0e-15, atol=1.0e-300): | |||
Test equality of all columns in a table, with stricter tolerances for | |||
float columns than the np.allclose default. | |||
""" | |||
assert_equal(len(t1), len(t2)) | |||
assert_equal(t1.colnames, t2.colnames) | |||
npt.assert_equal(len(t1), len(t2)) |
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.
Here npt.assert_equal()
is being used to check lengths of lists. Compare the tracebacks:
============================================== FAILURES ===============================================
__________________________________________ test_example_good __________________________________________
def test_example_good():
> assert len(list1) == len(list2)
E assert 2 == 3
E + where 2 = len([1, 2])
E + and 3 = len([1, 2, 3])
example.py:8: AssertionError
__________________________________________ test_example_bad ___________________________________________
def test_example_bad():
> np.testing.assert_equal(len(list1), len(list2))
E AssertionError:
E Items are not equal:
E ACTUAL: 2
E DESIRED: 3
example.py:11: AssertionError
assert_equal(len(t1), len(t2)) | ||
assert_equal(t1.colnames, t2.colnames) | ||
npt.assert_equal(len(t1), len(t2)) | ||
npt.assert_equal(t1.colnames, t2.colnames) |
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.
Here npt.assert_equal()
is being used to check columns of tables. Compare the tracebacks:
============================================== FAILURES ===============================================
__________________________________________ test_example_good __________________________________________
def test_example_good():
> assert t1.colnames == t2.colnames
E AssertionError: assert ['a', 'b', 'c'] == ['a', 'c']
E At index 1 diff: 'b' != 'c'
E Left contains one more item: 'c'
E Use -v to get more diff
example.py:11: AssertionError
__________________________________________ test_example_bad ___________________________________________
def test_example_bad():
> np.testing.assert_equal(t1.colnames, t2.colnames)
E AssertionError:
E Items are not equal:
E ACTUAL: 3
E DESIRED: 2
example.py:14: AssertionError
Those are scalar/string comparisons. I did not expect the older helper functions would even have come into play there, but it looks like originally they were substitutes for unittest functions. I agree in those cases it makes no sense to bring |
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.
Actually it seems the majority of equal
s are just comparing scalars or simple strings; so it might be better to default to assert
first and then search for array-like and structured objects where to better stick with assert_equal
.
if check_meta: | ||
assert_equal(t1.meta, t2.meta) | ||
npt.assert_equal(t1.meta, t2.meta) |
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.
This is a bit trickier, as meta is neither a sequence nor a scalar, but typically a dict-like structure.
But here the numpy function should still provide better diagnostics:
>>> assert t1.meta == t2.meta
AssertionError
>>> assert_equal(t1.meta, t2.meta)
AssertionError:
Items are not equal:
key='value'
key='CLIPRANGE'
key='keywords'
ACTUAL: '2.5'
DESIRED: '2.0'
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.
The tracebacks the CPython interpreter produces can be very different from the tracebacks pytest
produces:
============================================== FAILURES ===============================================
__________________________________________ test_example_good __________________________________________
def test_example_good():
> assert t1.meta == t2.meta
E AssertionError: assert {'CLIPRANGE':... 'value': 2.5} == {'CLIPRANGE':... 'value': 2.0}
E Omitting 2 identical items, use -vv to show
E Differing items:
E {'value': 2.5} != {'value': 2.0}
E Use -v to get more diff
example.py:11: AssertionError
__________________________________________ test_example_bad ___________________________________________
def test_example_bad():
> np.testing.assert_equal(t1.meta, t2.meta)
E AssertionError:
E Items are not equal:
E key='value'
E
E ACTUAL: 2.5
E DESIRED: 2.0
example.py:15: AssertionError
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.
Yes, the pytest version might be a bit more useful here still. Would you suggest to leave numpy's assert_equal
to array-like objects only and use assert ...==
for everything else?
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.
When in doubt, make the test fail and see what the pytest
traceback looks like. But for this pull request the reasonable thing to do would be to replace assert_equal()
from commons.py
with assert ... == ...
, not np.testing.assert_equal()
. commons.assert_equal(x, y)
is basically an alias for assert x == y
anyways, so that replacement will not make any potential tracebacks worse.
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.
@dhomeier Should I start working on replacing the npt.assert_equal
back to assert x == y
.
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.
We have discussed in the meantime that npt
is not really a standard alias used by astropy, so
- those imports should be changed to
numpy.testing import assert_allclose, assert_equal
in general - for comparing anything that is not an array-like collection of numerical values (i.e. basically anything that can be converted to a standard numerical-dtype
ndarray
), per the suggestions above, replaceassert_equal
withassert x == y
.
Thanks!
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.
Sure, will start my work as mentioned
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.
Just to clarify, we want
assert_equal(arr_x, arr_y)
assert obj_x == obj_y
… assert . Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
…the marked TODOs. Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
assert_true((data["a"].mask == [False, True]).all()) | ||
assert_true((data["a"] == [1, 1]).all()) | ||
assert_true((data["b"].mask == [False, True]).all()) | ||
assert_true((data["b"] == [2, 1]).all()) | ||
assert (data["a"].mask == [False, True]).all() | ||
assert (data["a"] == [1, 1]).all() | ||
assert (data["b"].mask == [False, True]).all() | ||
assert (data["b"] == [2, 1]).all() |
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.
In case of an error we would get a better traceback with numpy.testing.assert_array_equal()
, but the current patch would not make the traceback any worse than it already is. I'll therefore leave it to the sub-package maintainers to decide if the checks should be further modified in this pull request or if there should be a separate follow-up.
I'm highlighting this only here, but there are more analogous checks elsewhere.
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Hello, @dhomeier a gentle reminder for a review. Thank you for your time. |
@@ -223,7 +218,7 @@ def test_read_all_files(fast_reader, path_format, home_is_data): | |||
table = ascii.read(testfile["name"], **test_opts) | |||
assert_equal(table.dtype.names, testfile["cols"]) |
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.
assert_equal(table.dtype.names, testfile["cols"]) | |
assert table.dtype.names == testfile["cols"] |
These are tuples of strings, so better compared with the standard assert
@@ -253,7 +248,7 @@ def test_read_all_files_via_table(fast_reader, path_format, home_is_data): | |||
table = Table.read(testfile["name"], format=format, **test_opts) | |||
assert_equal(table.dtype.names, testfile["cols"]) |
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.
assert_equal(table.dtype.names, testfile["cols"]) | |
assert table.dtype.names == testfile["cols"] |
as above
@@ -448,7 +443,7 @@ def process_lines(lines): | |||
reader.inputter.process_lines = process_lines | |||
data = reader.read("data/bars_at_ends.txt") | |||
assert_equal(data.dtype.names, ("obsid", "redshift", "X", "Y", "object", "rad")) |
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.
assert_equal(data.dtype.names, ("obsid", "redshift", "X", "Y", "object", "rad")) | |
assert data.dtype.names == ("obsid", "redshift", "X", "Y", "object", "rad") |
assert (data["a"].mask == [False, True]).all() | ||
assert (data["a"] == [1, 1]).all() | ||
assert (data["b"].mask == [False, True]).all() | ||
assert (data["b"] == [2, 1]).all() |
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.
assert (data["a"].mask == [False, True]).all() | |
assert (data["a"] == [1, 1]).all() | |
assert (data["b"].mask == [False, True]).all() | |
assert (data["b"] == [2, 1]).all() | |
assert_equal(data["a"].mask, [False, True]) | |
assert_equal(data["a"], [1, 1]) | |
assert_equal(data["b"].mask, [False, True]) | |
assert_equal(data["b"], [2, 1]) |
I agree the numpy.testing
function provides rather better diagnostics here even though the arrays are small.
This applies to all the assert (...).all()
checks here and below.
In contrast, all the assert_equal
uses above this line (basically all comparisons of some dtype.names
are comparing tuples of non-numeric objects and should use assert ==
– not just those in the suggestions.
@@ -29,9 +28,9 @@ def test_read_normal(): | |||
reader = ascii.get_reader(reader_cls=ascii.RST) | |||
dat = reader.read(table) | |||
assert_equal(dat.colnames, ["Col1", "Col2"]) |
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.
assert_equal(dat.colnames, ["Col1", "Col2"]) | |
assert dat.colnames == ["Col1", "Col2"] |
Here as well, all the "names" comparisons are better done this way
@@ -12,6 +12,7 @@ | |||
import numpy as np | |||
import pytest | |||
from numpy import ma | |||
from numpy.testing import assert_allclose, assert_equal |
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.
This looks half-finished. As you saw, we can just inline assert_equal
instead of replacing it with numpy.testing.assert_equal
. A couple call sites seem to remain, let's smoke them out !
from numpy.testing import assert_allclose, assert_equal | |
from numpy.testing import assert_allclose |
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.
This looks half-finished. As you saw, we can just inline
assert_equal
instead of replacing it withnumpy.testing.assert_equal
. A couple call sites seem to remain, let's smoke them out !
@neutrinoceros there has been an ongoing discussion from
#15839 (comment), #15839 (comment) and following about when to better use simple assert
vs. Numpy's assert_equal
on array-like operands. I am afraid there is no uncontroversial golden path where to use what, but my last set of comments and suggestion was trying to find a reasonable balance of using both options. Do you think assert x == y
should be always preferred even on numerical ndarrays?
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.
Oh, sorry I missed it !
Do you think assert x == y should be always preferred even on numerical ndarrays?
no ! I was trying to be strictly conservative in my review, but I agree that this pattern is best avoided for ndarrays !
@@ -42,24 +41,24 @@ def assert_table_equal(t1, t2, check_meta=False, rtol=1.0e-15, atol=1.0e-300): | |||
Test equality of all columns in a table, with stricter tolerances for | |||
float columns than the np.allclose default. | |||
""" | |||
assert_equal(len(t1), len(t2)) | |||
assert len(t1) == len(t2) | |||
assert_equal(t1.colnames, t2.colnames) |
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.
assert_equal(t1.colnames, t2.colnames) | |
assert t1.colnames == t2.colnames |
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.
there's another one on line 1136
@@ -10,6 +10,7 @@ | |||
|
|||
import numpy as np | |||
import pytest | |||
from numpy.testing import assert_allclose, assert_equal |
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.
There are still 16 calls to assert_equal in this file to clean up (some of them already pointed out by @dhomeier)
from numpy.testing import assert_allclose, assert_equal | |
from numpy.testing import assert_allclose |
@@ -3,13 +3,12 @@ | |||
from io import StringIO | |||
|
|||
import numpy as np | |||
from numpy.testing import assert_allclose, assert_equal |
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.
7 occurrences left to clean up here
from numpy.testing import assert_allclose, assert_equal | |
from numpy.testing import assert_allclose |
@Harsh-pandya are you still available to finish this effort? |
Change Description:
Performed removals
Description
This pull request is to address ...
Fixes #15782