astropy
astropy copied to clipboard
Cosmo as a dataclass
This PR changes Cosmology and its subclasses to dataclasses.dataclass objects for versions py3.10+.
Why change?
Well, this has long been the intended plan. Cosmology is structured very similarly to the dataclass paradigm (see cosmology.Parameter) but small differences meant that they were not compatible.
Dataclasses bring a number of advantages over normal classes, particularly for classes that are defined by their attributes (so not dict-like things).
- straightforward declaration of the fields / attributes
- no need to define most dunder methods.
- static typing compatibility
- support for match statements
- easy to make immutable structures
- introspection tools like
fieldsandasdict, etc. - easier to define dynamically define subclasses
- deep support in transpilation / compilation libraries, e.g. mypyc.
- ...
- [x] 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.
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.
👋 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?
v6.1 is fine I guess.
https://numpy.org/neps/nep-0029-deprecation_policy.html says "On Apr 05, 2024 drop support for Python 3.9 (initially released on Oct 05, 2020)". Astropy v6.1 won't be released until after April, right? So we can bump our python version for v6.1, making this PR significantly simpler. Ping @saimn @pllim for a sense of our release schedule
Yes, "major/minor" release is still every 6 months or so. See https://github.com/astropy/astropy-APEs/blob/main/APE2.rst#detailed-description
It would be best to put this on hold until support for Python 3.9 is dropped.
Ok. Then we can bump Python for v6.1 and get this in then.
https://github.com/astropy/astropy/wiki/Release-Calendar needs some updates but yes, 6.1 should happen early May 2024.
BTW, if you want to bump Python, please do that in a separate PR, not here. Thanks!
Rebased on #15706 and #15603. when those are merged I'll re-mark this PR as ready for review.
Furthermore, and out of curiosity, can I ask for a short explanation of what makes Python 3.10's data classes better for existing code ?
dataclasses.dataclass objects have a number of distinct advantages
- Less code => reduced code debt
- Natural definition of immutability
- Better introspection (annotations and tools from
dataclasses) - Better I/O (tools from
dataclasses) - Enforced good code structure.
- ensure compatibility with
mypyctranspilation.
Except for the time required to upgrade the code I submit the opposite, why would we not make a class a dataclass?
why would we not make a class a dataclass?
A concern I heard about data classes is startup time overhead.
There are also alternatives like attrs, which is historically a precursor to data classes, but it is still actively developed as a third party package.
But I don't think we need to worry about the startup overhead too much for a library, and I just mentioned attrs for good measure, but if we can live happily with the standard lib implementation, no need to reach out for another third party dependency !
@eerovaher @WilliamJamieson @neutrinoceros, now that #15603 is in, this PR is ready as well! It will be great to finally upgrade the architecture of Cosmology. This should make a lot of other features easier to implement.
@eerovaher @WilliamJamieson, the feature freeze is coming up. What do y'all think of this PR?
I agree we should try to get this merged before the feature freeze. I'll make time to have a look at it tomorrow. Luckily @neutrinoceros has already reviewed it, so I don't expect that any big changes are going to be needed.
Do you plan editing the commit history yourself? Currently there are two different commits that both Make Cosmology a dataclass, so a normal merge doesn't seem to be the right thing to do, but you don't want this to be automatically squashed either.
Yes, this is a product of an interactive rebase. I'll fix it up.
🎉 thanks all for the reviews!!