astropy icon indicating copy to clipboard operation
astropy copied to clipboard

Cosmo as a dataclass

Open nstarman opened this issue 2 years ago • 11 comments

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

  1. straightforward declaration of the fields / attributes
  2. no need to define most dunder methods.
  3. static typing compatibility
  4. support for match statements
  5. easy to make immutable structures
  6. introspection tools like fields and asdict, etc.
  7. easier to define dynamically define subclasses
  8. deep support in transpilation / compilation libraries, e.g. mypyc.
  9. ...
  • [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.

nstarman avatar Oct 15 '23 00:10 nstarman

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[bot] avatar Oct 15 '23 00:10 github-actions[bot]

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

github-actions[bot] avatar Oct 15 '23 00:10 github-actions[bot]

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

nstarman avatar Oct 29 '23 16:10 nstarman

Yes, "major/minor" release is still every 6 months or so. See https://github.com/astropy/astropy-APEs/blob/main/APE2.rst#detailed-description

pllim avatar Oct 30 '23 14:10 pllim

It would be best to put this on hold until support for Python 3.9 is dropped.

eerovaher avatar Oct 30 '23 16:10 eerovaher

Ok. Then we can bump Python for v6.1 and get this in then.

nstarman avatar Oct 30 '23 17:10 nstarman

https://github.com/astropy/astropy/wiki/Release-Calendar needs some updates but yes, 6.1 should happen early May 2024.

saimn avatar Oct 30 '23 17:10 saimn

BTW, if you want to bump Python, please do that in a separate PR, not here. Thanks!

pllim avatar Oct 30 '23 19:10 pllim

Rebased on #15706 and #15603. when those are merged I'll re-mark this PR as ready for review.

nstarman avatar Dec 15 '23 16:12 nstarman

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

  1. Less code => reduced code debt
  2. Natural definition of immutability
  3. Better introspection (annotations and tools from dataclasses)
  4. Better I/O (tools from dataclasses)
  5. Enforced good code structure.
  6. ensure compatibility with mypyc transpilation.

Except for the time required to upgrade the code I submit the opposite, why would we not make a class a dataclass?

nstarman avatar Jan 11 '24 01:01 nstarman

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 !

neutrinoceros avatar Jan 11 '24 07:01 neutrinoceros

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

nstarman avatar Mar 10 '24 16:03 nstarman

@eerovaher @WilliamJamieson, the feature freeze is coming up. What do y'all think of this PR?

nstarman avatar Mar 26 '24 19:03 nstarman

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.

eerovaher avatar Mar 27 '24 18:03 eerovaher

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.

nstarman avatar Mar 28 '24 20:03 nstarman

🎉 thanks all for the reviews!!

nstarman avatar Mar 29 '24 17:03 nstarman