Skip to content

Cosmo parameters format - #12983

Closed
nstarman wants to merge 1 commit into
astropy:mainfrom
nstarman:cosmo-parameters-format
Closed

Cosmo parameters format#12983
nstarman wants to merge 1 commit into
astropy:mainfrom
nstarman:cosmo-parameters-format

Conversation

@nstarman

@nstarman nstarman commented Mar 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Cosmology Parameters now have _format_spec_types to store format-specific
representations of a Parameter. While private API, this PR will make #12356 and #12355 much easier.

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

@nstarman nstarman added this to the v5.1 milestone Mar 23, 2022
@pep8speaks

pep8speaks commented Mar 23, 2022 •
edited
Loading

Copy link
Copy Markdown

Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2022-04-06 14:57:20 UTC

@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 9cddad4 to a684fd9 Compare March 23, 2022 18:52
@nstarman
nstarman requested a review from mhvk March 23, 2022 18:53
@github-actions

github-actions Bot commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

👋 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 force-pushed the cosmo-parameters-format branch 5 times, most recently from eb6a996 to 60caae0 Compare March 23, 2022 22:36
@nstarman
nstarman marked this pull request as ready for review March 23, 2022 23:58
@nstarman
nstarman requested a review from adrn March 24, 2022 14:46
adrn
adrn reviewed Mar 24, 2022
View reviewed changes

@adrn adrn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From a first (quick) look, without digesting the context too much, it seems a little confusing to me to have fmt and format as keyword arguments -- is there a way to combine these?

Comment thread astropy/utils/collections.py Resolved
@nstarman
nstarman requested review from adrn and removed request for adrn March 24, 2022 17:19
@nstarman
nstarman force-pushed the cosmo-parameters-format branch 4 times, most recently from 8903638 to e848267 Compare March 28, 2022 20:24
adrn
adrn reviewed Mar 29, 2022
View reviewed changes
Comment thread astropy/cosmology/parameter.py Resolved
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from e848267 to 60caae0 Compare March 30, 2022 18:16
adrn
adrn approved these changes Mar 30, 2022
View reviewed changes

@adrn adrn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool - this looks good to me now! My only remaining question is whether utils/collections.py is the right place to put this. I defer to @pllim to comment on that :)

@nstarman

nstarman commented Mar 30, 2022

Copy link
Copy Markdown
Member Author

@adrn @pllim. I think we might want a place in utils to have base classes and mixins that span multiple sub-packages.
_DisplayFormatAttributeMixin is one example. Another is that astropy rolls its own instance-level descriptors in a few different places and it would be good to unify that with a common base class.

@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 377bc2a to 36883a5 Compare March 31, 2022 21:22
@nstarman

nstarman commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

In #12921 we introduce a new file, utils/abcs.py. @pllim, should there be a utils/mixins.py for general mixin classes?

@nstarman nstarman mentioned this pull request Apr 2, 2022
Merged
10 tasks done
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from f7edab8 to 6b41413 Compare April 2, 2022 16:39
@nstarman nstarman mentioned this pull request Apr 2, 2022
Closed
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from f48b520 to 968972d Compare April 6, 2022 14:21
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 968972d to 6858d77 Compare April 6, 2022 14:57
@mhvk

mhvk commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

Sorry, no time until next week!

@nstarman nstarman mentioned this pull request Apr 11, 2022
Merged
10 tasks
@nstarman nstarman modified the milestones: v5.1, v5.2 Apr 15, 2022
mhvk
mhvk reviewed Apr 26, 2022
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Am a bit torn about this, as I don't particularly like the units interface/naming. Some comments in-line.

Comment thread astropy/cosmology/parameter.py Resolved
Comment thread astropy/cosmology/tests/test_parameter.py Resolved
Comment thread astropy/cosmology/tests/test_parameter.py Resolved
Comment thread astropy/utils/mixins.py Resolved
Comment thread astropy/utils/mixins.py Resolved
Comment thread astropy/utils/mixins.py Resolved
@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Sep 4, 2022
@github-actions

github-actions Bot commented Sep 4, 2022

Copy link
Copy Markdown
Contributor

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@nstarman nstarman added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Sep 8, 2022
@nstarman

nstarman commented Sep 16, 2022

Copy link
Copy Markdown
Member Author

I'm going to take another look at this and probably split this into 2 PRs.

  1. Just implement it for Cosmology
  2. Try to unify the Unit and Cosmology implementations.

@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 6858d77 to 62684db Compare September 17, 2022 17:29
@nstarman
nstarman requested a review from a team as a code owner September 17, 2022 17:29
@mhvk mhvk removed this from the v5.2 milestone Oct 16, 2022
@mhvk

mhvk commented Oct 16, 2022

Copy link
Copy Markdown
Contributor

Clearing the milestone since the last comment was that this would be split in 2 PRs.

@nstarman nstarman added this to the v6.0 milestone Oct 17, 2022
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 62684db to f7180dd Compare October 19, 2022 21:30
@nstarman

nstarman commented Oct 19, 2022

Copy link
Copy Markdown
Member Author

Starting with a private implementation.

@pllim
pllim marked this pull request as draft October 19, 2022 21:36
@pllim

pllim commented Oct 19, 2022

Copy link
Copy Markdown
Member

I converted this to draft so this won't accidentally go into v5.2. Thanks for your understanding!

@nstarman
nstarman force-pushed the cosmo-parameters-format branch 2 times, most recently from 44cfbcd to 73d6486 Compare May 17, 2023 23:12
@nstarman
Add format_spec type info to Parameter class
bcb60b8 
This is entirely private API.

Signed-off-by: Nathaniel Starkman <[email protected]>
@nstarman
nstarman force-pushed the cosmo-parameters-format branch from 73d6486 to bcb60b8 Compare May 18, 2023 01:28
@nstarman nstarman closed this Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrn adrn adrn approved these changes
@pllim pllim pllim left review comments
@mhvk mhvk mhvk left review comments

Assignees

No one assigned

Labels

cosmology keep-open no-changelog-entry-needed units utils

Projects

Cosmology, the Expansion
Status: Done

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nstarman @pep8speaks @mhvk @pllim @adrn