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

Replaced the usage of custom function assert_equal with np.testing.assert_equal #15839

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Harsh-pandya
Copy link

@Harsh-pandya Harsh-pandya commented Jan 10, 2024

Change Description:

Performed removals

  • assert_equal()
  • assert_almost_equal()
  • assert_true()

Description

This pull request is to address ...

Fixes #15782

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

… with np.testing.assert_equal

Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Copy link

github-actions bot commented Jan 10, 2024

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Copy link

@github-actions github-actions bot left a 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.

Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

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>
@Harsh-pandya Harsh-pandya marked this pull request as ready for review January 10, 2024 04:18
@Harsh-pandya
Copy link
Author

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?

  1. Check if towncrier change log entry is correct

  2. Precommit.ci
    ruff.....................................................................Failed

    • hook id: ruff
    • exit code: 1
    • files were modified by this hook
    astropy/io/ascii/tests/test_c_reader.py:54:25: PT009 Use a regular `assert` instead of unittest-style `assert_`
    

    Over here the function provided by np.testing is assert_. This function is the replacement for the assert_true in common.py.

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.
Screenshot 2024-01-09 at 7 06 22 PM

Copy link
Contributor

@neutrinoceros neutrinoceros left a 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

  1. from numpy.testing import assert_array_equal
  2. import numpy.testing as npt

Comment on lines 22 to 24


# Compatibility functions to convert from nose to pytest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Compatibility functions to convert from nose to pytest

Comment just related to the removed functions, so this can all be deleted.

@dhomeier
Copy link
Contributor

2. Over here the function provided by np.testing is assert_. This function is the replacement for the assert_true in common.py.

I think the idea was rather to replace those with a simple assert as in

                        assert not isinstance(t2[name][i], str) and np.isnan(t2[name][i]

According to the docs npt.assert_ seems only useful for optimised code, and I don't think we ever need that in our unit tests.

@dhomeier
Copy link
Contributor

Just a general comment on style: in astropy, numpy testing function are usually imported one of the following ways

  1. from numpy.testing import assert_array_equal
  2. import numpy.testing as npt

In addition, using the former for assert_almost_equal would allow to leave test bodies essentially unmodified.

As for assert_equal I am not sure if the intention was to use anything at all but a simple assert a == b
@eerovaher , do the better failure diagnostics you mentioned in #15782 apply here as well (if one does not want to specify a custom error message)?

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.

@dhomeier
Copy link
Contributor

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.

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.

@pllim
Copy link
Member

pllim commented Jan 10, 2024

Gotta do something about that pre-commit rule.

@dhomeier
Copy link
Contributor

dhomeier commented Jan 10, 2024

You mean remove the rule? 🤔
PT009 does not seem to have any clue about numpy's assert_, so maybe that is something to consider, even if we are not testing in optimised mode atm.

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?

@pllim
Copy link
Member

pllim commented Jan 10, 2024

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!

@eerovaher
Copy link
Member

@dhomeier asked

As for assert_equal I am not sure if the intention was to use anything at all but a simple assert a == b –
@eerovaher , do the better failure diagnostics you mentioned in #15782 apply here as well (if one does not want to specify a custom error message)?

It's usually best to use the numpy.testing helpers for checking array-like collections of numerical values, with the possible exception of checking strict equality, but for anything else I think pytest introspection is better. I am not aware of any good reasons to use np.testing.assert_(). Have a look at these examples:

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:

============================================== FAILURES ===============================================
___________________________________________ test_example_1 ____________________________________________

    def test_example_1():
>   	assert [1, 3, 3] == values
E    assert [1, 3, 3] == [1, 2, 3]
E      At index 1 diff: 3 != 2
E      Use -v to get more diff

test_example.py:8: AssertionError
___________________________________________ test_example_2 ____________________________________________

    def test_example_2():
>       assert np.array_equal([1, 3, 3], values)
E       assert False
E        +  where False = <function array_equal at 0x7f3c07d43eb0>([1, 3, 3], [1, 2, 3])
E        +    where <function array_equal at 0x7f3c07d43eb0> = np.array_equal

test_example.py:12: AssertionError
___________________________________________ test_example_3 ____________________________________________

    def test_example_3():
>       np.testing.assert_array_equal([1, 3, 3], values)

test_example.py:16: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (<built-in function eq>, [1, 3, 3], [1, 2, 3])
kwds = {'err_msg': '', 'header': 'Arrays are not equal', 'strict': False, 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Arrays are not equal
E           
E           Mismatched elements: 1 / 3 (33.3%)
E           Max absolute difference: 1
E           Max relative difference: 0.5
E            x: array([1, 3, 3])
E            y: array([1, 2, 3])

/usr/lib/python3.10/contextlib.py:79: AssertionError
___________________________________________ test_example_4 ____________________________________________

    def test_example_4():
>   	np.testing.assert_([1, 3, 3] == values)
E    AssertionError

test_example.py:20: AssertionError

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:

============================================== FAILURES ===============================================
___________________________________________ test_example_1 ____________________________________________

    def test_example_1():
>   	assert "Expectd String  output." == message
E    AssertionError: assert 'Expectd String  output.' == 'Expected string output.'
E      - Expected string output.
E      ?       -  ^
E      + Expectd String  output.
E      ?         ^     +

test_example.py:8: AssertionError
___________________________________________ test_example_2 ____________________________________________

    def test_example_2():
>       np.testing.assert_array_equal("Expectd String  output.", message)

test_example.py:12: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (<built-in function eq>, 'Expectd String  output.', 'Expected string output.')
kwds = {'err_msg': '', 'header': 'Arrays are not equal', 'strict': False, 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Arrays are not equal
E           
E           Mismatched elements: 1 / 1 (100%)
E            x: array('Expectd String  output.', dtype='<U23')
E            y: array('Expected string output.', dtype='<U23')

/usr/lib/python3.10/contextlib.py:79: AssertionError
___________________________________________ test_example_3 ____________________________________________

    def test_example_3():
>   	np.testing.assert_("Expectd String  output." == message)
E    AssertionError

test_example.py:16: AssertionError

…rts for numpy testing package. Also removed the unused import in common.py file.

Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
@dhomeier
Copy link
Contributor

It's usually best to use the numpy.testing helpers for checking array-like collections of numerical values, with the possible exception of checking strict equality, but for anything else I think pytest introspection is better. I am not aware of any good reasons to use np.testing.assert_(). Have a look at these examples:

Thanks; I don't see much benefit in assert assert np.array_equal(), but np.testing.assert_array_equal certainly offers more useful info especially for large arrays!
I suppose assert np.testing.assert_ may only become useful if one creates a custom msg, but since I don't see a clear case for testing .pyo files, I'd leave it at assert there (and keep PT008 enabled).

Copy link
Contributor

@dhomeier dhomeier left a 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!

Comment on lines 8 to 13
import numpy as np
import numpy.testing as npt
import pytest
from contextlib import nullcontext
from io import BytesIO
from textwrap import dedent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member

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.

Comment on lines 54 to 56
npt.assert_(
not isinstance(t2[name][i], str) and np.isnan(t2[name][i])
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@dhomeier
Copy link
Contributor

Sorry I have my college work so I am unable to work on it

No problem, as we would rather go with the alternative approach (using the numpy.testing functions where possible) here. Thank you for you work so far @Rohit-Pujari, and hope this will give you time to set up a working development installation for your next contribution!

@dhomeier dhomeier changed the title Replaced the usage of function called asset_equal with np.testing.assert_equal Replaced the usage of custom function assert_equal with np.testing.assert_equal Jan 13, 2024
Copy link
Contributor

@dhomeier dhomeier left a 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!

@eerovaher
Copy link
Member

In most if not all cases we want to replace the old assert_equal() with assert ... == ... instead of npt.assert_equal().

@dhomeier
Copy link
Contributor

I understood your comparison in #15839 (comment)

It's usually best to use the numpy.testing helpers for checking array-like collections of numerical values,

to exactly the opposite effect – assert_equal providing more detailed info (in fact, imo, a bit more useful yet than assert_array_equal for plain lists etc.):

>>> np.testing.assert_equal([1, 3, 3], [1, 2, 3])
AssertionError: 
Items are not equal:
item=1

 ACTUAL: 3
 DESIRED: 2

Copy link
Member

@eerovaher eerovaher left a 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))
Copy link
Member

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)
Copy link
Member

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

@dhomeier
Copy link
Contributor

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 numpy.testing in at all, so they will all have to be reviewed case by case.

Copy link
Contributor

@dhomeier dhomeier left a 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 equals 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)
Copy link
Contributor

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'

Copy link
Member

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

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Author

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.

Copy link
Contributor

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

  1. those imports should be changed to numpy.testing import assert_allclose, assert_equal in general
  2. 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, replace assert_equal with assert x == y.
    Thanks!

Copy link
Author

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

Copy link
Contributor

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

@dhomeier dhomeier mentioned this pull request Jan 17, 2024
1 task
Harsh-pandya and others added 3 commits January 27, 2024 19:24
… assert .

Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
…the marked TODOs.

Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
astropy/io/ascii/tests/test_c_reader.py Outdated Show resolved Hide resolved
Comment on lines -546 to +544
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()
Copy link
Member

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.

astropy/io/ascii/tests/test_c_reader.py Outdated Show resolved Hide resolved
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
Signed-off-by: Harsh-pandya <harshpandya735@gmail.com>
@Harsh-pandya
Copy link
Author

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"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal(data.dtype.names, ("obsid", "redshift", "X", "Y", "object", "rad"))
assert data.dtype.names == ("obsid", "redshift", "X", "Y", "object", "rad")

Comment on lines +541 to +544
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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 !

Suggested change
from numpy.testing import assert_allclose, assert_equal
from numpy.testing import assert_allclose

Copy link
Contributor

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 !

@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?

Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal(t1.colnames, t2.colnames)
assert t1.colnames == t2.colnames

Copy link
Contributor

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
Copy link
Contributor

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)

Suggested change
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
Copy link
Contributor

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

Suggested change
from numpy.testing import assert_allclose, assert_equal
from numpy.testing import assert_allclose

@dhomeier
Copy link
Contributor

dhomeier commented Apr 4, 2024

@Harsh-pandya are you still available to finish this effort?

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 needless functions from an io.ascii test helper file
8 participants