Cosmological units module #12092

nstarman · 2021-08-20T01:50:34Z

Adds a new module to Cosmology — units — for defining and collecting cosmological units and
equivalencies.
@eteq, though I advocated for it, I'm holding off from actually moving littleh and with_H0 for now. The latter will be easy to move, but moving littleh and having it be imported into units/astrophys is a nightmare of circular imports.
It would be much easier to just deprecate it from units/astrophys, but I'd want some discussion on that, and since it's a low priority, I'll save it for a followup. Thanks!

Necessary For : #11784
Fixes: #12093

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

pep8speaks · 2021-08-20T01:50:39Z

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 2021-08-24 19:33:17 UTC

mhvk

This looks good. A question is whether the documentation of with_H0 in units.equivalencies should not just point here. But perhaps that becomes more logical if the equivalency and unit definitions are moved here as well - which is perhaps requires more logic than is worth it...

Anyway, more important is to move forward and define new stuff here!

docs/cosmology/index.rst

mhvk · 2021-08-23T17:26:31Z

docs/cosmology/index.rst

@@ -557,3 +566,4 @@ Reference/API
 =============

 .. automodapi:: astropy.cosmology
+


Extraneous empty line? (Emacs would auto-remove it with my no-end-of-line space setting...)

Ah. My editor has an end-of-line space setting! 😆.
I think it's PEP8 W292 (https://www.flake8rules.com/rules/W292.html)

mhvk · 2021-08-23T17:27:33Z

astropy/cosmology/units.py

+# This generates a docstring for this module that describes all of the
+# standard units defined here.
+if __doc__ is not None:
+    __doc__ += _generate_unit_summary(globals())


Would it be an idea to define enable() (as in imperial.py, etc.)?

Let me take a look! that seems a good idea.

My second thought was perhaps not (see below), but do have your own look, since clearly I am wavering!

docs/cosmology/units.rst

nstarman · 2021-08-24T06:35:57Z

@mhvk, I'm not sure if we want to deprecate littleh and with_H0 from astropy.units, so I've collapsed that into one commit. Can roll back...

eerovaher · 2021-08-24T11:41:15Z

docs/cosmology/units.rst

+"hours."
+
+If no argument is given (or the argument is `None`), this equivalency assumes
+the ``H0`` from the current default :ref:`cosmology <astropy-cosmology>`:


When this text was in the units documentation it made sense to have a link to the cosmology subpackage, but if you are moving the text then it might be better to use

Suggested change

the ``H0`` from the current default :ref:`cosmology <astropy-cosmology>`:

the ``H0`` from the current default :class:`~astropy.cosmology.Cosmology`:

Thanks! applied in 958aa5c

mhvk

I was unsure, but now seeing it, I think a real move to cosmology.units and deprecating in units is the right way to go! Also, LTS versions are the best place to do these types of things, as then they have more minimal impact for packages that try to support LTS+current.

astropy/cosmology/units.py

astropy/units/__init__.py

mhvk · 2021-08-24T17:08:52Z

docs/changes/cosmology/12092.feature.rst

@@ -0,0 +1,3 @@
+Added units module for defining and collecting cosmological units and
+equivalencies. The units can be added to the main Astropy units namespace using


As you noted, this is not quite true.

I've removed that portion.

For some future date: with a module-level__getattr__ and the units registry it should be possible for add_enabled_units to also put the units on the top-level namespace in a way that's compatible with _UnitsContext (context manager returned by add_enabled_units).

True, though perhaps too much magic? And perhaps worries for thread safety? I guess the idea of enabling units was mostly that they can be used in strings, and thus in storing them inside files, etc.

docs/cosmology/units.rst

docs/units/index.rst

astropy/units/astrophys.py

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@nstarman

Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

mhvk

Looks all great now, except still to revert the added lines in docs/units/index.rst (I think).

docs/units/index.rst

mhvk · 2021-08-24T19:22:16Z

docs/units/index.rst

@@ -252,6 +253,7 @@ Reference/API
 .. automodapi:: astropy.units.physical

 .. automodapi:: astropy.units.equivalencies
+   :skip: with_H0


And this one too?

How did those sneak in there?!? I used a fixup! commit to prune them from the whole history

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@nstarman

Co-authored-by: Eero Vaher <eero.vaher@astro.lu.se> Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman · 2021-08-24T19:35:38Z

Thanks @mhvk and @eerovaher for the reviews!

mhvk

Looks all OK now! (Weird about the git thingy not working)

mhvk · 2021-08-24T19:59:18Z

The code coverage is clearly a red herring, so merging. Thanks, @nstarman!

nstarman · 2021-08-24T21:05:37Z

@mhvk. I'm thinking this might have merited a whatsnew... here or in the followup adding cosmological redshift units

mhvk · 2021-08-24T21:20:31Z

Yes, with the redshift unit - so that it makes sense that there is a new cosmology.units module.

In astropy#12092 `littleh` and `with_H0` were moved from `units` to `cosmology.units`, but importing them from `units` was still possible, albeit deprecated. Removing them from `units` entirely finalizes the moving process.

nstarman added this to the v5.0 milestone Aug 20, 2021

nstarman requested a review from eteq August 20, 2021 01:50

nstarman added this to In progress in Cosmology, the Expansion via automation Aug 20, 2021

github-actions bot added the cosmology label Aug 20, 2021

nstarman force-pushed the cosmo_units branch from f7044d2 to 942e230 Compare August 20, 2021 01:51

nstarman added the units label Aug 20, 2021

nstarman force-pushed the cosmo_units branch from 942e230 to 94ebcbe Compare August 20, 2021 01:58

nstarman changed the title ~~Cosmo units~~ Cosmological units module Aug 20, 2021

nstarman force-pushed the cosmo_units branch from 94ebcbe to 070938a Compare August 20, 2021 04:41

mhvk reviewed Aug 23, 2021

View reviewed changes

nstarman force-pushed the cosmo_units branch from 070938a to 15e3d64 Compare August 23, 2021 18:47

eerovaher reviewed Aug 23, 2021

View reviewed changes

docs/cosmology/units.rst Show resolved Hide resolved

nstarman force-pushed the cosmo_units branch 6 times, most recently from bf9af7a to 3a44376 Compare August 24, 2021 06:23

nstarman force-pushed the cosmo_units branch from 3a44376 to 041c0de Compare August 24, 2021 06:42

eerovaher reviewed Aug 24, 2021

View reviewed changes

nstarman force-pushed the cosmo_units branch from 041c0de to 958aa5c Compare August 24, 2021 16:03

mhvk reviewed Aug 24, 2021

View reviewed changes

nstarman and others added 2 commits August 24, 2021 14:43

Collect cosmological units and equivalencies

df2405f

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

Document unit: littleh

a281dc7

Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the cosmo_units branch 2 times, most recently from 588c0bf to f3f3c00 Compare August 24, 2021 19:14

mhvk reviewed Aug 24, 2021

View reviewed changes

move definitions to cosmology

3603b9a

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the cosmo_units branch from f3f3c00 to 3e9c891 Compare August 24, 2021 19:26

nstarman and others added 2 commits August 24, 2021 15:32

deprecate littleh and with_H0 from units

9c707a2

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

copy editing

6be1b4f

Co-authored-by: Eero Vaher <eero.vaher@astro.lu.se> Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the cosmo_units branch from 3e9c891 to 6be1b4f Compare August 24, 2021 19:33

mhvk approved these changes Aug 24, 2021

View reviewed changes

Cosmology, the Expansion automation moved this from In progress to Reviewer approved Aug 24, 2021

mhvk merged commit 2631ed5 into astropy:main Aug 24, 2021

Cosmology, the Expansion automation moved this from Reviewer approved to Done Aug 24, 2021

nstarman deleted the cosmo_units branch August 24, 2021 20:04

nstarman mentioned this pull request Aug 24, 2021

Cosmological Redshift Units #11786

Merged

eerovaher mentioned this pull request Oct 20, 2023

Remove deprecated functionality from units #15514

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmological units module #12092

Cosmological units module #12092

nstarman commented Aug 20, 2021 •

edited

pep8speaks commented Aug 20, 2021 •

edited

mhvk left a comment

mhvk Aug 23, 2021

nstarman Aug 23, 2021

mhvk Aug 23, 2021

nstarman Aug 23, 2021

mhvk Aug 23, 2021

nstarman commented Aug 24, 2021

eerovaher Aug 24, 2021

nstarman Aug 24, 2021

mhvk left a comment

mhvk Aug 24, 2021

nstarman Aug 24, 2021

mhvk Aug 24, 2021

mhvk left a comment

mhvk Aug 24, 2021

nstarman Aug 24, 2021

nstarman Aug 24, 2021

nstarman commented Aug 24, 2021

mhvk left a comment

mhvk commented Aug 24, 2021

nstarman commented Aug 24, 2021

mhvk commented Aug 24, 2021

		@@ -557,3 +566,4 @@ Reference/API
		=============

		.. automodapi:: astropy.cosmology

	the ``H0`` from the current default :ref:`cosmology <astropy-cosmology>`:
	the ``H0`` from the current default :class:`~astropy.cosmology.Cosmology`:

		@@ -0,0 +1,3 @@
		Added units module for defining and collecting cosmological units and
		equivalencies. The units can be added to the main Astropy units namespace using

Cosmological units module #12092

Cosmological units module #12092

Conversation

nstarman commented Aug 20, 2021 • edited

Checklist for package maintainer(s)

pep8speaks commented Aug 20, 2021 • edited

Comment last updated at 2021-08-24 19:33:17 UTC

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstarman commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstarman commented Aug 24, 2021

mhvk left a comment

Choose a reason for hiding this comment

mhvk commented Aug 24, 2021

nstarman commented Aug 24, 2021

mhvk commented Aug 24, 2021

nstarman commented Aug 20, 2021 •

edited

pep8speaks commented Aug 20, 2021 •

edited