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.

Sign up for GitHub

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

Jump to bottom

Remove needless functions from an io.ascii test helper file #15782

Open
3 tasks
eerovaher opened this issue Dec 22, 2023 · 6 comments · May be fixed by #15787, #15839, #16266 or #16383
Open
3 tasks

Remove needless functions from an io.ascii test helper file #15782

eerovaher opened this issue Dec 22, 2023 · 6 comments · May be fixed by #15787, #15839, #16266 or #16383
Labels
io.ascii testing

Comments

@eerovaher
Copy link
Member

eerovaher commented Dec 22, 2023 •
edited

What is the problem this feature will solve?

A helper file in io.ascii tests contains three functions that serve little purpose:

astropy/astropy/io/ascii/tests/common.py

Lines 27 to 37 in fefe226

# Compatibility functions to convert from nose to pytest
def assert_equal(a, b):
assert a == b
def assert_almost_equal(a, b, **kwargs):
assert np.allclose(a, b, **kwargs)
def assert_true(a):
assert a

Describe the desired outcome

The aforementioned functions should be removed and any tests still using them should be updated accordingly. Pull requests targeting one or more functions are welcome.

Additional context

assert_equal() and assert_true() should be inlined, but assert_almost_equal(a, b) should be replaced with np.testing.assert_allclose(a, b) instead of assert np.allclose(a, b) because the dedicated testing function produces a better error message if the check fails.

Performed removals

  • assert_equal()
  • assert_almost_equal()
  • assert_true()
@eerovaher eerovaher added testing io.ascii Package-novice good first issue Issues that are well-suited for new contributors labels Dec 22, 2023
@pllim
Copy link
Member

pllim commented Dec 22, 2023

Looks like @taldcroft added these a long time ago. Are other io.ascii maintainers on board with this? I don't see any link to previous discussions.

@taldcroft
Copy link
Member

Fine by me. I would expect there was a reason for these functions in the days of yore, but I don't see a reason now.

@Rohit-Pujari
Copy link

Can i solve this issue? (i am a beginner so i will try)

@pllim
Copy link
Member

pllim commented Dec 23, 2023

@Rohit-Pujari , thank you for your interest! There is no need to ask for permission. But please do read through https://docs.astropy.org/en/latest/index_dev.html if you are new.

@Rohit-Pujari
Copy link

i have raised the pull request,please inform me what i have did wrong

@pllim pllim linked a pull request Dec 23, 2023 that will close this issue
Open
1 task
@Harsh-pandya Harsh-pandya linked a pull request Jan 10, 2024 that will close this issue
Open
4 tasks
@vagish-yadav
Copy link

Hi is this issue still open to resolve !!??

@eerovaher eerovaher mentioned this issue Feb 21, 2024
Open
1 task
stefanarseneau added a commit to stefanarseneau/astropy that referenced this issue Apr 3, 2024
@stefanarseneau
resolving issue astropy#15782
0a77151 
replaced all calls of:
    assert_equal() -> np.testing.assert_equal()
    assert_almost_equal() -> np.testing.assert_allclose()
    assert_true(a) -> assert a
in the following files:
    astropy/io/ascii/tests/test_c_reader.py
    astropy/io/ascii/tests/test_cds.py
    astropy/io/ascii/tests/test_rst.py
    astropy/io/ascii/tests/test_types.py
    astropy/io/ascii/tests/test_read.py
    astropy/io/ascii/tests/test_fixedwidth.py
    astropy/io/ascii/tests/test_cds_header_from_readme.py
@stefanarseneau stefanarseneau linked a pull request Apr 3, 2024 that will close this issue
Open
1 task
@helloworld-chhanda helloworld-chhanda linked a pull request May 4, 2024 that will close this issue
Open
4 tasks
@pllim pllim removed Package-novice good first issue Issues that are well-suited for new contributors labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
io.ascii testing
Projects
None yet
Milestone
No milestone
5 participants
@taldcroft @pllim @eerovaher @Rohit-Pujari @vagish-yadav