Cosmology property is_flat

[nstarman]

Signed-off-by: Nathaniel Starkman (@nstarman) nstarkman@protonmail.com

Description

Add property is_flat to cosmologies to verify the curvature of the Universe.

It's abstract on Cosmology, making it abstract (finally).
On FLRW it checks Ok0 and Otot0.
On subclassess of FlatCosmologyMixin it's just True.

Flatness is a subtle thing. On FLRW cosmologies it's simple, but not in general. So it's abstract on Cosmology.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• 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 "When to rebase and squash commits".
• 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.
• 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 a milestone set? Milestone must be set but astropy-bot check might be missing; 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?

[nstarman]

I still need to add a what's-new and update some doc, but the code is ready to review.

[mhvk]

Looks good except I would perhaps suggest not to make it an abstract property; it just seems to add quite a bit of boilerplate, especially calling super just to get NotImplementedError back again. It also makes for a real API break - user-defined classes now have to change. (I've more generally found the concept of these abstract methods a bit less useful than I thought initially; I've not see a clear win with it.)

But really only a suggestion - if you think this is not a real problem, that's fine too.

[mhvk]

This makes me wonder a little whether the abstractproperty really is all that useful.

[nstarman]

it just seems to add quite a bit of boilerplate, especially calling super just to get NotImplementedError back again. It also makes for a real API break - user-defined classes now have to change.

It does add a bit of boilerplate, but crucially all that boilerplate is in the test code. I cannot find on GH any example of someone directly subclassing Cosmology, the base class, which would cause an API break and add boilerplate to user code. Our test code requires the boilerplate since it is testing directly subclassing Cosmology.
FLRW is actually the effective base class since all built-in cosmologies are its subclasses; and FLRW is necessarily abstract because of w(z). So we are in this awkward situation where the true base class is useless and not abstract and the useful 'base' class is abstract.
I modeled is_flat on w(z), including its slightly odd "double" abstraction: erroring if the method isn't overwritten and raising NotImplementedError if called. I actually kind of like this construction. The abstractmethod indicates subclasses must define the method, the NotImplementedError means one cannot just super their way around that. The latter is how the tests are checking that Cosmology is correctly abstract.

But really only a suggestion - if you think this is not a real problem, that's fine too.

I think I'll keep it as is. Thanks again for the detailed and quick review!

[nstarman]

Don't merge yet, I'm still working on the docs.

Done. Added a what's-new.