Support running with -OO

[alchzh]

Description

This pull request supports the -OO/PYTHONOPTIMIZE=2 options by

• Removing uses of assert outside of tests (and enabling the corresponding ruff rule)
• Adding missing foo.__doc__ is not None checks around docstring code
• Testing the above by allowing the test suite to run with -OO (skipping docstring/help() tests)

Fixes #15052

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

[github-actions]

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

[alchzh]

Coverage is failing because of assert replaces- will try to add test cases for the new errors.

I've chosen to replace asserts with descriptive errors (ValueError, TypeError, etc. with messages) where relevant, although this may break user code with explicit except AssertionError checks. Should we use explicit raise AssertionError for strict backward compatibility instead?

[pllim]

Re: #15789 (comment)

@nstarman , do you know since you ported the config to pyproject.toml a while ago?

[alchzh]

It was failing earlier when I made that comment but it looks like it's passing now (though there's still an annotation that doesn't look right). Should this get a single changelog entry at the top level or multiple for each submodule?

[mhvk]

Since for the user of regular python, there is no behaviour change, I think it should be a top-level feature one that python -OO is now possible (of course, it will no longer be any faster, at least from skipping assert statements...).

[alchzh]

Since for the user of regular python, there is no behaviour change, I think it should be a top-level feature one that python -OO is now possible (of course, it will no longer be any faster, at least from skipping assert statements...).

Are any of these asserts in extremely hot areas that would have a non-microscopic impact on performance? We should keep those as asserts if so. I think the main advantage to -OO is just for compatibility (since it seems like some people are running programs with -OO that include astropy) with the added benefit of some memory savings from removing the the docstrings.

[mhvk]

No, they aren't in hot paths generally. And, yes, of course good for people who run with -OO that it works!

[nstarman]

Thanks for the PR! I only looked at the cosmology changes, which all look good.
My more general comment is to put "TODO:" markers in your comments about replacing AssertionErrors.

[alchzh]

codecov failures look spurious to me- and they seem to change randomly despite no changes to the code.

[mhvk]

Yes, I've sadly learned to just ignore code coverage entirely...

[nstarman]

The patch coverage is pretty good. It's the project coverage that's problematic.

[eerovaher]

I have a few remarks regarding the changes to pyproject.toml.

[eerovaher]

It is not obvious to me that we should permanently ignore S101 in all of docs/ just because of two files in docs/wcs/examples/.

[nstarman]

Cosmology & Units: LGTM. Thanks @alchzh.

[eerovaher]

Here's what the violations in docs/ look like (in current main):

$ ruff check --select S101 --show-source docs/
docs/wcs/examples/from_file.py:46:5: S101 Use of `assert` detected
   |
44 |     # These should be the same as the original pixel coordinates, modulo
45 |     # some floating-point error.
46 |     assert np.max(np.abs(pixcrd - pixcrd2)) < 1e-6
   |     ^^^^^^ S101
47 | 
48 |     # The example below illustrates the use of "origin" to convert between
   |

docs/wcs/examples/from_file.py:54:5: S101 Use of `assert` detected
   |
52 |     y = 0
53 |     origin = 0
54 |     assert (w.wcs_pix2world(x, y, origin) ==
   |     ^^^^^^ S101
55 |             w.wcs_pix2world(x + 1, y + 1, origin + 1))
   |

docs/wcs/examples/programmatic.py:41:1: S101 Use of `assert` detected
   |
39 | # These should be the same as the original pixel coordinates, modulo
40 | # some floating-point error.
41 | assert np.max(np.abs(pixcrd - pixcrd2)) < 1e-6
   | ^^^^^^ S101
42 | 
43 | # The example below illustrates the use of "origin" to convert between
   |

docs/wcs/examples/programmatic.py:49:1: S101 Use of `assert` detected
   |
47 | y = 0
48 | origin = 0
49 | assert (w.wcs_pix2world(x, y, origin) ==
   | ^^^^^^ S101
50 |         w.wcs_pix2world(x + 1, y + 1, origin + 1))
   |

Found 4 errors.

We might want to replace these assert statements with np.testing.assert_allclose() and np.testing.assert_array_equal() instead of ignoring S101 in docs/.

[mhvk]

That's an option, though I must admit I don't quite like how the checkers here are affecting good & clear code. I somewhat prefer to leave the warts with the checker set-up files rather than with the code, i.e., either just exclude docs -- where presumably the author, like here, made a conscious choice to use assert -- or just exclude these two files.

p.s. This PR is to support running python with -OO - for that purpose, none of this should really matter...

[eerovaher]

As far as I can tell docs/wcs/examples/from_file.py exists only so that docs/wcs/loading_from_fits.rst can include it. But the latter contains only 8 lines in total. Perhaps it would be best to move the contents of from_file.py to loading_from_fits.rst. Likewise for docs/wcs/examples/programmatic.py and docs/wcs/example_create_imaging.rst.

[alchzh]

For now, I will move the exclude tule for docs to ruff.toml (just to make the CI happy) and maybe a separate PR can be made if the example files need to be modified or moved.

[eerovaher]

That's reasonable.