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

[helloworld-chhanda]

Description

This pull request is to address ...

Fixes #15782

Description:
assert_equal , assert_true has been removed from "common.py" as well as other files.
assert_allclose has been used instead of assert_almost_equal.

Performed removals

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

• 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.

[github-actions]

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?
• 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.

[github-actions]

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.

[eerovaher]

I had a quick look at the changes to io.ascii and it seems like only a couple of minor changes are needed there. However, a pull request addressing #15782 has no reason to edit anything outside io.ascii, so all the changes elsewhere need to be undone.

[eerovaher]

The functions should be deleted, not commented out.

[helloworld-chhanda]

Thank you for your feedback. The commented code has been removed in the recent commit.

[eerovaher]

Ruff correctly detects an F631 (assert-tuple) violation here. Our developer documentation recommends using the pre-commit tool, which would automatically detect such mistakes before a commit was even made.

Suggested change

	assert (
	table["loggf"].description ==
	"log10 of the gf value - logarithm base 10 of stat. weight times "
	"oscillator strength",
	)
	assert (
	table["loggf"].description ==
	"log10 of the gf value - logarithm base 10 of stat. weight times "
	"oscillator strength"
	)

[eerovaher]

I'm not sure if I would use this npt alias, but it is used in some of our tests already, so there is precedent for this.

[dhomeier]

–1 on adopting this. We have discussed this in one of the various PRs addressing the same issue before, and already concluded that from numpy.testing import assert_allclose etc. is the preferred style (and already used with only the aforementioned 1 or 2 exceptions).

[pllim]

Maybe we should remove "package-novice" label from #15782

[dhomeier]

Also should note this in #16266 or ping @stefanarseneau on a response to #16266 (comment).

[pllim]

We have at least 4 PRs now trying to address that issue and the diff here is too messy. I think we should close this without merge.

• Remove needless functions from an io.ascii test helper file #15782 issue sloved #15787
• Replaced the usage of custom function assert_equal with np.testing.assert_equal #15839
• Bugfix for issue 15782: Remove needless functions from an io.ascii test helper file #16266
• Removed needless functions from an io.ascii test helper file #15782 #16383 (this PR)

[eerovaher]

Out of all the related pull requests this seems to be closest to completion. The diff is not overly messy. It is far too large because of all the changes outside io.ascii, but undoing those changes is very simple and does not require review.

[eerovaher]

To undo all the changes outside io.ascii in your local repository the following might be useful:

$ git reset --hard $(git merge-base main @)
$ git restore --source=@{u} astropy/io/ascii

EDIT: the above instructions will not work in your case because you have opened the pull request from the main branch of your fork, not from a feature branch. Our developer documentation recommends that you name the main astropy repository astropy, in which case you should run

$ git reset --hard $(git merge-base astropy/main @)
$ git restore --source=@{u} astropy/io/ascii

But if in your local repository the main astropy repository is called origin then you should run

$ git reset --hard $(git merge-base origin/main @)
$ git restore --source=@{u} astropy/io/ascii