Cosmo as a dataclass by nstarman · Pull Request #15484 · astropy/astropy

nstarman · 2023-10-15T00:34:45Z

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.
built in static typing compatibility
support for match statements
easy to make immutable structures
introspection tools like fields and asdict, etc.
easier to dynamically define subclasses
deep support in transpilation / compilation libraries, e.g. mypyc.
...

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 · 2023-10-15T00:35:24Z

github-actions · 2023-10-15T00:35:28Z

👋 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 · 2024-01-11T01:05:12Z

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 mypyc transpilation.

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

neutrinoceros · 2024-01-11T07:37:02Z

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 !

nstarman · 2024-03-10T16:26:41Z

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

neutrinoceros

I'll set aside some time to finish this review tomorrow, in the mean time here are a couple (very) small questions.

neutrinoceros · 2024-03-12T17:42:02Z

        Any
            Results of evaluating the Cosmology method.
        """
+        # TODO: speed up using ``replace``


Should this be done in this PR ? Otherwise, why not ?

It's just out of scope. This function has to comply with requirements for making Astropy Models.

neutrinoceros

This time I was able to do a complete read through. This looks outstanding !

A broad (but very minor) comment is that I'm not sure about the name dataclass_decorator. How about maybe something more descriptive of its application domain like cosmology_dataclass ?

nstarman · 2024-03-10T16:21:04Z

        # Derived quantities:
        # Dark matter density; matter - baryons, if latter is not None.
-        self._Odm0 = None if Ob0 is None else (self._Om0 - self._Ob0)
+        object.__setattr__(


I want to collect all these object.__setattr__ ancillary variables into a more efficient storage object. But I think that should be done in a future PR.

neutrinoceros

With your latest batch of replies, I'm happy to sign this off !

nstarman · 2024-03-26T19:22:06Z

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

eerovaher · 2024-03-27T18:16:00Z

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

I do have a few comments, but it should be possible to address them before the feature freeze.

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.

eerovaher · 2024-03-28T16:24:16Z

        all_vars["name"].__set__(self, name)
        all_vars["meta"].__set__(self, OrderedDict(meta or {}))

+    def __post_init__(self):  # noqa: B027


The current docstring is very strongly suggesting that this should be an abstract method (which would also get rid of the noqa), so the docstring should be rewritten.

eerovaher · 2024-03-28T16:46:03Z

 )
 from astropy.cosmology.parameter._core import MISSING

 ##############################################################################


I think he meant line 19.

eerovaher · 2024-03-28T17:08:55Z

+now a :func:`~dataclasses.dataclass`. This means that the :mod:`dataclasses` machinery
+can be used to work with :class:`~astropy.cosmology.Cosmology` objects. For example::
+
+.. doctest-skip::


Why are the doctests being skipped?

nstarman · 2024-03-28T20:47:42Z

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.

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

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

py3.13+ will have the `converter` arg, eliminating this class. Co-authored-by: Clément Robert <[email protected]> Signed-off-by: nstarman <[email protected]>

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

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

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

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

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

nstarman · 2024-03-29T17:13:28Z

🎉 thanks all for the reviews!!

nstarman added Enhancement API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Oct 15, 2023

nstarman added this to the v6.0 milestone Oct 15, 2023

github-actions Bot added testing Docs cosmology labels Oct 15, 2023

nstarman force-pushed the cosmo-cosmo-dataclass branch 9 times, most recently from 6be68d6 to ee557a6 Compare October 16, 2023 13:37

nstarman force-pushed the cosmo-cosmo-dataclass branch 13 times, most recently from 5e03f6a to 6728751 Compare October 24, 2023 00:31

mhvk mentioned this pull request Jan 10, 2024

ascii.mrt format does not keep column names consistently #12804

Closed

nstarman mentioned this pull request Jan 15, 2024

make ParametersAttribute.attr_name a string #15882

Merged

neutrinoceros reviewed Mar 12, 2024

View reviewed changes

neutrinoceros reviewed Mar 13, 2024

View reviewed changes

nstarman commented Mar 13, 2024

View reviewed changes

neutrinoceros approved these changes Mar 13, 2024

View reviewed changes

eerovaher reviewed Mar 28, 2024

View reviewed changes

nstarman and others added 14 commits March 28, 2024 20:52

Fix Parameter tests using all_cls_vars

7e3eb91

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

Make Cosmology a dataclass

962d298

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

Make name a dataclass field

432ccb8

py3.13+ will have the `converter` arg, eliminating this class. Co-authored-by: Clément Robert <[email protected]> Signed-off-by: nstarman <[email protected]>

Make meta a dataclass field

afda42d

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

Make FLRW et al dataclasses

ca2321a

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

Use post-init

db72457

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

Freeze Cosmology dataclass

5b9e1dd

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

make Parameter compatible with dynamic init

b65a152

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

Freeze Cosmology dataclass

c29b367

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

use dataclass init

727a758

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

Simplify Parameter property creation

3d6ce80

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

Simplify cloning

6d6d350

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

refactor private method _init_signature

0bd84ac

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

use dataclass repr

7642cdd

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

eerovaher approved these changes Mar 29, 2024

View reviewed changes

nstarman mentioned this pull request Mar 29, 2024

Change Table and Column meta to dict and enforce YAML output as ordered map #16250

Merged

1 task

Conversation

nstarman commented Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 15, 2023

Uh oh!

github-actions Bot commented Oct 15, 2023

Uh oh!

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

Uh oh!

Uh oh!

neutrinoceros commented Jan 11, 2024

Uh oh!

nstarman commented Mar 10, 2024

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

nstarman Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman Mar 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman commented Mar 26, 2024

Uh oh!

eerovaher commented Mar 27, 2024

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerovaher Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eerovaher Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nstarman commented Mar 28, 2024

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

nstarman commented Oct 15, 2023 •

edited

Loading

nstarman commented Jan 11, 2024 •

edited

Loading

nstarman commented Mar 29, 2024 •

edited

Loading