Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cosmo param dataclass #14874

Merged
merged 5 commits into from Aug 2, 2023
Merged

Cosmo param dataclass #14874

merged 5 commits into from Aug 2, 2023

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented May 29, 2023 •
edited

Description

Cosmology Parameters are now dataclass.
Why? b/c dataclasses are nice:

  • clean, modern code & syntax
  • convenient how it writes the dunder methods
  • more introspectable, using tools from the dataclasses module.
  • work with dataclasses's functions
  • easy to build subclasses.

In this PR:

  • Add a compatibility flag for python 3.10
  • Refactored Parameter to be a dataclass
    • Due to the nature of dataclases, the private attribute _registry_validators had to be factored off of the class.
    • Got to remove the _init_arguments because dataclasses have replace!
    • Changed fvalidate from the processed value to the original one. This is a breaking change.

Future note: in py3.13+ https://peps.python.org/pep-0712/ will allow the UnitField class to be replaced with the converter argument to field(). In the meantime, it's a private dataclass field descriptor.

@nstarman nstarman added this to the v6.0 milestone May 29, 2023
@github-actions
Copy link

github-actions bot commented May 29, 2023 •
edited by eerovaher

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 "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. 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
Copy link

👋 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 nstarman mentioned this pull request May 29, 2023
Merged
@nstarman nstarman force-pushed the cosmo-param-dataclass branch 5 times, most recently from 93b2346 to 03b0d96 Compare May 29, 2023 20:17
@nstarman nstarman force-pushed the cosmo-param-dataclass branch 2 times, most recently from 04b9efe to 7d985dd Compare June 7, 2023 19:22
@nstarman nstarman marked this pull request as ready for review June 10, 2023 21:30
@nstarman nstarman requested a review from a team as a code owner June 10, 2023 21:30
@nstarman nstarman requested review from WilliamJamieson, mhvk and eerovaher and removed request for a team June 10, 2023 21:30
@nstarman nstarman force-pushed the cosmo-param-dataclass branch 2 times, most recently from a4ebe9c to 527a039 Compare June 20, 2023 20:12
@nstarman
Copy link
Member Author

This PR is ready, except that the docs are failing because they can't find the docstring for some descriptors.
@pllim @mhvk, any suggestions? I found from poking around on numpydoc that you've both tackled weird sphinx docs failures...

@pllim
Copy link
Member

pllim commented Jun 20, 2023 

cosmology/parameter/_core.py:docstring of astropy.cosmology.Parameter.fvalidate:1:
WARNING: py:class reference target not found: astropy.cosmology.parameter._core.FValidateField

cosmology/parameter/_core.py:docstring of astropy.cosmology.Parameter.unit:1:
WARNING: py:class reference target not found: astropy.cosmology.parameter._core.UnitField

Probably because you have no docstring for those attributes and Sphinx is trying to auto-generate something but then is choking because of private module?

    # Units
    unit: UnitField = UnitField()  # noqa: RUF009
    fvalidate: FValidateField = FValidateField(default="default")  # noqa: RUF009

@nstarman
Copy link
Member Author

nstarman commented Jun 20, 2023 •
edited

Probably because you have no docstring for those attributes and Sphinx is trying to auto-generate something but then is choking because of private module?

I used to have docstrings, but removed them in
527a039 because it gives the same error! I think it's something to do with how the generated docstring is being linked to the class of the attribute?

@pllim
Copy link
Member

pllim commented Jun 20, 2023

I think it's something to do with how the generated docstring is being linked to the class of the attribute?

Alas, I am not familiar with this particular error. You can try look in modeling and see how they do it.

@nstarman nstarman force-pushed the cosmo-param-dataclass branch 2 times, most recently from bbaa007 to 49d67b9 Compare June 22, 2023 18:47
@nstarman
Copy link
Member Author

Thanks! figured it out. Sphinx doesn't like semi-public descriptors. UnitField -> _UnitField fixes everything.

@nstarman
Copy link
Member Author

nstarman commented Jun 22, 2023 •
edited

@WilliamJamieson @mhvk @eerovaher, I got all the checks passing!

@nstarman nstarman force-pushed the cosmo-param-dataclass branch 2 times, most recently from 99445f9 to d91d796 Compare July 21, 2023 18:15
@nstarman nstarman requested a review from eerovaher July 22, 2023 19:16
@nstarman nstarman force-pushed the cosmo-param-dataclass branch 2 times, most recently from aded5a3 to 0825681 Compare July 28, 2023 02:16
@nstarman
Add PYTHON_LT_3_10 flag
5464996 
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman
Make Parameter a dataclass
b4a8371 
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman
Make name a field
a489c7f 
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman
simplify cloning Parameter
a32cdf9 
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman
Copy link
Member Author

nstarman commented Jul 30, 2023 •
edited

@eerovaher, RTD is rendering the attribute descriptions. https://astropy--14874.org.readthedocs.build/en/14874/api/astropy.cosmology.Parameter.html#astropy.cosmology.Parameter

eerovaher
eerovaher requested changes Jul 31, 2023
View reviewed changes
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have two small remarks left. But perhaps it is best to wait until #15065 or #15096 have been merged so that the CI tests wouldn't fail because of the new numpy scalar representations.

astropy/cosmology/parameter/_core.py Show resolved Hide resolved
docs/whatsnew/6.0.rst Outdated Show resolved Hide resolved
@nstarman nstarman mentioned this pull request Jul 31, 2023
Open
@nstarman nstarman requested a review from eerovaher August 1, 2023 01:31
eerovaher
eerovaher requested changes Aug 1, 2023
View reviewed changes
docs/whatsnew/6.0.rst Outdated Show resolved Hide resolved
@nstarman nstarman requested a review from eerovaher August 1, 2023 18:25
eerovaher
eerovaher requested changes Aug 2, 2023
View reviewed changes
Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff on GitHub looks good enough to merge, but when I looked at the What's New page on RTD I noticed that the new entry is not under the right section. Other than that this looks good to go.

@nstarman @eerovaher
changelog and whatsnew
898ad6f 
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
@nstarman
Copy link
Member Author

nstarman commented Aug 2, 2023 •
edited

The pains of rebasing! Thanks for noticing.
Fixed!

@nstarman nstarman requested a review from eerovaher August 2, 2023 19:34
@eerovaher eerovaher merged commit cd9dc7f into astropy:main Aug 2, 2023
23 of 25 checks passed
@nstarman nstarman deleted the cosmo-param-dataclass branch August 2, 2023 21:14
@nstarman
Copy link
Member Author

nstarman commented Aug 2, 2023

Thank you @eerovaher for the many valuable reviews!

@nstarman nstarman mentioned this pull request Aug 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerovaher eerovaher eerovaher approved these changes

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson

@mhvk mhvk Awaiting requested review from mhvk

@astrofrog astrofrog Awaiting requested review from astrofrog

Assignees
No one assigned
Labels
cosmology Refactoring utils whatsnew-needed
Projects
Cosmology, the Expansion
Status: Done
Milestone
v6.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nstarman @pllim @mhvk @eerovaher