cosmo: refactor layout, deprecate semi-private modules, add new io module by nstarman · Pull Request #17543 · astropy/astropy

nstarman · 2024-12-13T16:34:42Z

@mhvk, as we discussed offline. I've tried to make this change as easy to review as possible by breaking it into a sequence of atomic commits. It looks like there's a lot of line diffs, but actually it's much smaller than that, it's just that doing e.g. core.py -> _src/core.py and adding a @deprecated(core.py) confuses GH into counting all the lines in the move towards the total.

This PR has 3 parts:

move(cosmo): X commits move code into a private walled garden, re-exporting them exactly in place. There should be NO impact on (semi-)public API.
deprecate(cosmo): X deprecates semi-private/semi-public modules. While Astropy states in certain places in the docs that these are private modules which may be moved, in practice they are public and people wrongly import from them directly. Therefore these commits deprecate their contents without removing them, which will happen for Astropy v8.
feat: X commits add something new and do impact the public API.

What does this refactor enable?

0 ambiguity about public / private API
Much easier for devs to refactor code / add experimental features / generally work on stuff without impacting the API
it makes it much easier to follow the principle of separation of concerns
makes it easier to have a clean code layout, e.g. avoiding imports inside the unit equivalencies by moving the definition of the equivalencies to a new file (51100b3)

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 · 2024-12-13T16:35:11Z

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.

nstarman · 2024-12-13T22:21:59Z

@mhvk everything is working except an annoying sphinx warning about default_cosmology not being found.
My sphinx is refusing to build the astropy docs, so if you could review this sans working docs build, that'd be great. I can find time later to wrangle the docs. Thanks!

nstarman · 2025-01-08T23:35:22Z

@mhvk, do you want me to edit the commit history in any way to make review easier?

neutrinoceros

Thank you for carefully splitting this PR into atomic commits, it made reviewing, while somewhat tedious[^1], all the less confusing ! I also really like how the PR itself is atomic in a somewhat different sense; the merge commit will bring together all moving parts in a single clean go.

All of this seems reasonable to me, and I can certainly get behind your initial motivation for it; it's been brought up time and time again that the public/private boundary in this code base isn't clear to all users and caused both confusion and despair from both sides of the community (end users and core devs).
My one important concern is about deprecation warnings: I think we should avoid making any promises about when these deprecation will end (you never know if you'll be able to keep it, and even when it's possible to, we can also forget about it). On the other hand, it would be helpful to specify the "since" part of these messages.
Deprecated modules' docstrings should also avoid promising 8.0 as a due date.

[1]: though I can easily imagine that it's orders of magnitudes less tedious to read than it was to craft !

neutrinoceros · 2025-01-10T14:37:06Z

astropy/cosmology/_src/tests/test_deprecated_modules.py

+        pass
+
+    with (
+        warnings.catch_warnings(),


could you add a comment here (and in following similar tests) to explain why this filtering is needed ?

Signed-off-by: nstarman <[email protected]>

nstarman · 2025-01-11T02:22:06Z

My sphinx still wont build docs! Tox, uv, all venvs fail. Perplexing.

neutrinoceros

Just a quick note to anyone interested: I was going to point out that deprecation warnings were lacking a stacklevel argument, but after experimentation I think only the default value (stacklevel=1) shows the entire message within the traceback, and nothing is gained from using a different value, so, approving as is, thank you !

Since I stepped in as a reviewer at the request of @nstarman in an attempt to relieve @mhvk's burden, I think we should at least wait for his blessing before we merge this.

mhvk · 2025-01-11T12:54:27Z

No need to wait for me - for cosmology I've just been trying to help out (and failed to here).

neutrinoceros · 2025-01-11T14:08:46Z

great ! now we "just" need to fix that RTD failure.
@pllim do you have any pointers ?

pllim · 2025-01-11T17:14:43Z

The diff is too big for me to make a reasonable suggestion. But warnings are caused by broken refs. Did you make these "private" and hence undocumented? If so, stop linking to them in the public docs.

<unknown>:21: WARNING: py:obj reference target not found: astropy.cosmology.io [ref.obj]
<unknown>:23: WARNING: py:obj reference target not found: astropy.cosmology.funcs [ref.obj]
<unknown>:23: WARNING: py:obj reference target not found: astropy.cosmology.parameter [ref.obj]
<unknown>:23: WARNING: py:obj reference target not found: astropy.cosmology.core [ref.obj]
<unknown>:23: WARNING: py:obj reference target not found: astropy.cosmology.flrw [ref.obj]
<unknown>:23: WARNING: py:obj reference target not found: astropy.cosmology.io [ref.obj]
docs/cosmology/realizations.rst:309:<autosummary>:1: WARNING: py:obj reference target not found: astropy.cosmology.realizations.default_cosmology [ref.obj]

nstarman · 2025-01-13T18:23:57Z

As a note: current failure is

/home/docs/checkouts/readthedocs.org/user_builds/astropy/conda/17543/lib/python3.11/site-packages/astropy/cosmology/_src/default.py:docstring of astropy.cosmology._src.default.default_cosmology:1: WARNING: duplicate object description of astropy.cosmology._src.default.default_cosmology, other instance in api/astropy.cosmology.default_cosmology, use :no-index: for one of them

What's mystifying is I have used :no-index:.

pllim · 2025-01-13T18:40:23Z

Message: 'Found additional options no-index in automodapi.'

I don't think :no-index: is compatible with automodapi either. 😬

pllim · 2025-01-13T18:41:12Z

I see that you attempted :no-index: in realization but I didn't see any for default_cosmology. Are they the same thing?

nstarman · 2025-01-18T20:11:37Z

Thanks @neutrinoceros for the review of this large PR!

astropy.cosmology._utils was renamed to astropy.cosmology._src.utils in astropy/astropy#17543.

nstarman added this to the v7.1.0 milestone Dec 13, 2024

nstarman requested a review from mhvk December 13, 2024 16:34

github-actions bot added testing Docs cosmology labels Dec 13, 2024

nstarman force-pushed the cosmo/mnt/src-layout branch 2 times, most recently from 1d0bf4f to 4cdf60a Compare December 13, 2024 16:56

astropy deleted a comment from github-actions bot Dec 13, 2024

nstarman force-pushed the cosmo/mnt/src-layout branch 2 times, most recently from 735d327 to d6fd077 Compare December 13, 2024 17:38

nstarman marked this pull request as ready for review December 13, 2024 17:51

nstarman requested a review from a team as a code owner December 13, 2024 17:51

nstarman marked this pull request as draft December 13, 2024 17:52

nstarman force-pushed the cosmo/mnt/src-layout branch 2 times, most recently from fd3876e to 3ea2634 Compare December 13, 2024 20:14

nstarman marked this pull request as ready for review December 13, 2024 22:21

nstarman mentioned this pull request Dec 18, 2024

Towards Pedagogical Cosmological Models #17150

Open

22 tasks

nstarman mentioned this pull request Jan 9, 2025

STY: autofix for PLR1716 (boolean-chained-comparison) #17616

Merged

1 task

neutrinoceros self-requested a review January 10, 2025 08:19

neutrinoceros requested changes Jan 10, 2025

View reviewed changes

nstarman added 7 commits January 10, 2025 19:21

feat: add private API src folder

37dd479

Signed-off-by: nstarman <[email protected]>

refactor: separate cosmology imports

fce8cdd

Signed-off-by: nstarman <[email protected]>

move(cosmo): move tests to private API

db8ba8b

Signed-off-by: nstarman <[email protected]>

move(cosmo): move signature_deprecations to private API

2c3bc00

Signed-off-by: nstarman <[email protected]>

move(cosmo): move setup_package to private API

10307dc

Signed-off-by: nstarman <[email protected]>

move(cosmo): move utils to private API

109d095

Signed-off-by: nstarman <[email protected]>

move(cosmo): move typing to private API

ca13c9a

Signed-off-by: nstarman <[email protected]>

nstarman added 13 commits January 10, 2025 19:21

move(cosmo): move core to private API

e535c7d

Signed-off-by: nstarman <[email protected]>

move(cosmo): move flrw to private API

c6addec

Signed-off-by: nstarman <[email protected]>

deprecate(cosmo): deprecate semi-private funcs module

9ec3ab1

Signed-off-by: nstarman <[email protected]>

deprecate(cosmo): deprecate semi-private parameter module

e4493a9

Signed-off-by: nstarman <[email protected]>

refactor(cosmo): move all_parameters to parameter module

4ca46f5

Signed-off-by: nstarman <[email protected]>

deprecate(cosmo): deprecate semi-private connect module

54ae980

Signed-off-by: nstarman <[email protected]>

deprecate(cosmo): deprecate semi-private core module

b32ec9f

Signed-off-by: nstarman <[email protected]>

deprecate(cosmo): deprecate flrw module

d6b1f01

Signed-off-by: nstarman <[email protected]>

feat(cosmo): public io module

07d6c33

Signed-off-by: nstarman <[email protected]>

refactor(cosmo): move imports out of equivalencies

0b25abf

Signed-off-by: nstarman <[email protected]>

docs: changelog

a3701f2

Signed-off-by: nstarman <[email protected]>

TST: docs no-index

3ec3fd4

Signed-off-by: nstarman <[email protected]>

refactor(deprecations): address review comments re module deprecations

ccab463

nstarman force-pushed the cosmo/mnt/src-layout branch from 43377ab to ccab463 Compare January 11, 2025 01:52

neutrinoceros approved these changes Jan 11, 2025

View reviewed changes

fix(cosmology): api docs and links

498e6cb

nstarman force-pushed the cosmo/mnt/src-layout branch from 26f4263 to 498e6cb Compare January 13, 2025 17:16

docs: update realization api reference

bfe78a7

nstarman merged commit a34c310 into astropy:main Jan 18, 2025
28 checks passed

nstarman deleted the cosmo/mnt/src-layout branch January 18, 2025 20:19

nstarman mentioned this pull request Jan 23, 2025

API: reintroduce private classes from astropy.visualization.lupton_rgb that were removed without deprecation in astropy 7.0.0 #17531

Merged

1 task

lpsinger added a commit to lpsinger/ligo.skymap that referenced this pull request May 4, 2025

Track rename of astropy.cosmology._utils

5147355

astropy.cosmology._utils was renamed to astropy.cosmology._src.utils in astropy/astropy#17543.

Uh oh!

Comments

Conversation

nstarman commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 13, 2024

Uh oh!

nstarman commented Dec 13, 2024

Uh oh!

nstarman commented Jan 8, 2025

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

nstarman commented Jan 11, 2025

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jan 11, 2025

Uh oh!

neutrinoceros commented Jan 11, 2025

Uh oh!

pllim commented Jan 11, 2025

Uh oh!

nstarman commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jan 13, 2025

Uh oh!

pllim commented Jan 13, 2025

Uh oh!

nstarman commented Jan 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nstarman commented Dec 13, 2024 •

edited

Loading

nstarman commented Jan 13, 2025 •

edited

Loading