Alternate constants API 2 by embray · Pull Request #745 · astropy/astropy

embray · 2013-02-07T23:57:07Z

This is the much promised followup to #572. It includes the changes from #572 in some form or another, plus many additional changes as discussed in that issue.

Now it is possible to do from astropy.constants import <const>. For example:

In [1]: from astropy.constants import G

In [2]: G
Out[2]: <Constant name='Gravitational constant' value=6.67384e-11 error=8e-15 units='m3 / (kg s2)' reference='CODATA 2010'>

Most constants can now be combined directly with other quantities and values as before:

In [10]: from astropy import units as u

In [11]: u.Quantity(1.2, 'solMass').to('kg') * G
Out[11]: <Quantity 1.59299221728e+20 m3 / (s2)>

It can also be converted to other systems of units with different bases:

In [12]: G.cgs
Out[12]: <Quantity 6.67384e-08 1.000000e+00 cm3 / (g s2)>

However, one of the primary motivations of these changes was to be able to deal sensibly with constants that are defined in units that are incompatible with each other between different systems of units--primarily SI and EM extensions to CGS, e (electron charge) being the primary example. In this case, it is possible to import e as we did with G:

In [13]: from astropy.constants import e

In [14]: e
Out[14]: <Constant name='Electron charge' value=1.602176565e-19 error=3.5e-27 units='C' reference='CODATA 2010'>

And as you can see, the value of e is given in Coulombs (SI). However, this is too ambiguous for general use. What if we try to combine e with some other quantity? The result could have completely different physical dimensions depending on which system we're working in. So for constants that are defined with incompatible units depending on the system it's necessary to specify a system in order to combine it with anything:

In [46]: 100 * e
ERROR: TypeError: Constant 'e' does not have physically compatible units across all systems of units and cannot be combined with other values without specifying a system (eg. e.esu) [astropy.constants.constant]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-46-b403ffc6e3ce> in <module>()
----> 1 100 * e

/bray/sc1/root/src/astropy/astropy/astropy/constants/constant.py in wrapper(self, *args, **kwargs)
     35                         'combined with other values without specifying a '
     36                         'system (eg. {1}.{2})'.format(self.abbrev, self.abbrev,
---> 37                                                       systems[0]))
     38 
     39                 return meth(self, *args, **kwargs)

TypeError: Constant 'e' does not have physically compatible units across all systems of units and cannot be combined with other values without specifying a system (eg. e.esu)

In [47]: 100 * e.si
Out[47]: <Quantity 1.602176565e-17 C>

Electromagnetic constants are further a special case, as there are multiple EM extensions to CGS:

In [3]: e.cgs
ERROR: TypeError: Cannot convert EM constants to cgs because there are different systems for E.M constants within the c.g.s system (ESU, Gaussian, etc.). Instead, directly use the constant with the appropriate suffix (e.g. e.esu, e.gauss, etc.). [astropy.constants.constant]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-ebc8d90fd7bd> in <module>()
----> 1 e.cgs

/bray/sc1/root/src/astropy/astropy/astropy/constants/constant.py in cgs(self)
    172     @property
    173     def cgs(self):
--> 174         raise TypeError("Cannot convert EM constants to cgs because there "
    175                         "are different systems for E.M constants within the "
    176                         "c.g.s system (ESU, Gaussian, etc.). Instead, "

TypeError: Cannot convert EM constants to cgs because there are different systems for E.M constants within the c.g.s system (ESU, Gaussian, etc.). Instead, directly use the constant with the appropriate suffix (e.g. e.esu, e.gauss, etc.).

In [4]: e.gauss
Out[4]: <Constant name='Electron charge' value=4.80320450571e-10 error=1.049273603e-17 units='Fr' reference='CODATA 2010'>

Although the original motivation for this was handling EM constants in CGS, this will also pave the way to implementing other incompatible systems of units, such as geometrized units.

Another major change made in this PR is that there is no longer a separate _constants hidden package. Constants are defined once, in the astropy.constants package. This PR shuffles things around in such a way as to carefully avoid any problems with the circular dependency between the constants and units subpackages (though it's a bit fragile, and at a later date we may wish to further revisit this--I think it might make sense to combine units and constants into a single package, while only maintaining the top-level astropy.units and astropy.constants namespaces for convenience).

astrofrog · 2013-02-08T10:11:46Z

It looks like some builds errored on Travis - there is one genuine failure which is due to a test using == to compare a floating point value:

961_____________________ test_angular_diameter_distance_z1z2 ______________________
962
963    @pytest.mark.skipif('not HAS_SCIPY')
964    def test_angular_diameter_distance_z1z2():
965        tcos = core.FlatLambdaCDM(70.4,0.272,Tcmb0=0.0)
966>       assert (tcos.angular_diameter_distance_z1z2(1, 2) ==
967                646.22968662822018)
968E       assert 646.22968662822007 == 646.2296866282202
969E        +  where 646.22968662822007 = <bound method FlatLambdaCDM.angular_diameter_distance_z1z2 of FlatLambdaCDM(H0=70.4, Om0=0.272, Ode0=0.728)>(1, 2)
970E        +    where <bound method FlatLambdaCDM.angular_diameter_distance_z1z2 of FlatLambdaCDM(H0=70.4, Om0=0.272, Ode0=0.728)> = FlatLambdaCDM(H0=70.4, Om0=0.272, Ode0=0.728).angular_diameter_distance_z1z2
971
972astropy/cosmology/tests/test_cosmology.py:400: AssertionError

This may happen because constants previously defined in a system are being converted from another system, but in any case, the test should probably use

from astropy.tests.compat import assert_allclose

It might be worth fixing that here to force the build to restart anyway (can't restart it manually for some reason).

astrofrog · 2013-02-08T10:33:21Z

@iguananaut - this is looking great! Took me a while to understand the code, but I see what it's doing now, and it's very smart! I think it's the right approach, so I'm going to give detailed comments.

I've run into a couple of issues while testing:

If trying e.g. e * 100 multiple times, the correct error is raised the first time, but another is then raised for subsequent calls:

In [1]: from astropy.constants import e

In [2]: e * 100
ERROR: TypeError: Constant 'e' does not have physically compatible units across all systems of units and cannot be combined with other values without specifying a system (eg. e.esu) [astropy.constants.constant]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-07d0aa10c7c9> in <module>()
----> 1 e * 100

/Volumes/Raptor/Library/Python/3.2/lib/python/site-packages/astropy-0.3.dev3190-py3.2-macosx-10.8-x86_64.egg/astropy/constants/constant.py in wrapper(self, *args, **kwargs)
     35                         'combined with other values without specifying a '
     36                         'system (eg. {1}.{2})'.format(self.abbrev, self.abbrev,
---> 37                                                       systems[0]))
     38 
     39                 return meth(self, *args, **kwargs)

TypeError: Constant 'e' does not have physically compatible units across all systems of units and cannot be combined with other values without specifying a system (eg. e.esu)

In [3]: e * 100
ERROR: UnboundLocalError: local variable 'instances' referenced before assignment [astropy.constants.constant]
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<ipython-input-3-07d0aa10c7c9> in <module>()
----> 1 e * 100

/Volumes/Raptor/Library/Python/3.2/lib/python/site-packages/astropy-0.3.dev3190-py3.2-macosx-10.8-x86_64.egg/astropy/constants/constant.py in wrapper(self, *args, **kwargs)
     29                 if (not self.system and
     30                         name_lower in Constant._has_incompatible_units):
---> 31                     systems = sorted([_f for _f in instances if _f])
     32                     raise TypeError(
     33                         'Constant {0!r} does not have physically compatible '

UnboundLocalError: local variable 'instances' referenced before assignment

I fixed this locally by moving the definition of instances outside the first if statement and it seems to work.

Should the __repr__ for EMConstant include EMConstant instead of Constant?
Should the __str__ include a list of available systems? (just an idea)

I'll add a few in-line comments.

astrofrog · 2013-02-08T10:34:55Z

The docstring above needs to be updated as it refers to the old API.

astrofrog · 2013-02-08T10:41:06Z

Apart from my comments above, the docs themselves need to be updated. Thanks for all the work you've put into this! The user API ends up being a lot nicer and intuitive :-)

mdboom · 2013-02-08T14:20:26Z

Looks good to me.

embray · 2013-02-08T15:12:53Z

I see what's going on with that UnboundLocalError I can fix that. I might actually just duplicate the assignment of instances. The idea there was to avoid too much overhead in that wrapper when it's not necessary. A couple if statements is one thing, but dictionary lookups, while still very fast, can be considered a fair bit of overhead when it's not necessary.

On the other hand, every attribute lookup is already a dictionary lookup, so adding one more doesn't make much of a difference and is probably a premature optimization sigh

embray · 2013-02-08T15:27:11Z

Regarding the __repr__ for EMConstant: I sort of see EMConstant as an implementation detail, and not something worth exposing to users, so it was deliberately not mentioned. But I don't know, what do you think?

astrofrog · 2013-02-08T15:28:13Z

@iguananaut - I don't have a strong opinion, so if you don't think it needs to be exposed to users, then that's fine by me.

embray · 2013-02-08T15:30:26Z

Listing the available systems is not a bad idea either. There's still a little incongruity in how this all works though that might be worth revisiting at a later time. If I define G in SI, then its default system is SI. If I do G.cgs I get back a Quantity because G is not explicitly defined anywhere in CGS--its units are just being converted. But if I do explicitly put a definition for G in the CGS module then G.cgs returns a Constant, which is the instance of G defined in the CGS module. Make sense?

In other words, it's possible to have Constant instances for any number of systems of units, but it isn't necessary to so long as the units can be directly converted.

In the future I think we'll want to further rethink the distinction between units and constants given that some units are defined in terms of some constant and vice versa. That's where we've been getting into the most trouble with this.

astrofrog · 2013-02-08T15:36:07Z

Ah I see, so cgs wouldn't be listed as a system that can be converted to (in the system framework). Maybe si and cgs could always be listed, unless it's an EMConstant? As I said, this is minor so I wouldn't hold back 0.2 over this, but if there's an easy implementation, it could be handy.

embray · 2013-02-08T15:50:43Z

Well it's designed so that all the systems it can be converted to will show up in the constant's dir() or with tab-completion. But I could maybe do something like that. si and cgs are only there by default anyways since they're properties on the Quantity class.

embray · 2013-02-08T16:10:20Z

Out of curiosity I traced the slight difference in that one cosmology test to a slight floating point error that occurred in the old version in converting c from SI to CGS and then to 'km/s' that introduced an error of 0.00000000004. The new value is actually closer to "correct". In any case the test should still use allclose there.

astrofrog · 2013-02-08T17:06:19Z

Ah, cool - I am kind of impressed we hadn't run into the floating point test failing before, given the wide range of architectures we've tested in (of course it should be fixed).

embray · 2013-02-08T17:08:18Z

I actually had seen that test failing before but kind of ignored it. Since no one else had reported it I assumed it was just something weird about my hand-built SciPy :)

embray · 2013-02-08T18:14:00Z

3363515 also fixed the failing cosmology test.

adrn · 2013-02-08T18:48:51Z

This is looking very nice from an API perspective -- haven't looked at the code yet, but thanks for the work @iguananaut !

embray · 2013-02-08T18:59:11Z

@adrn Great--let me know (and this goes to anyone else) if there are any parts in the code that need to be clarified with further comments.

astrofrog · 2013-02-08T20:27:50Z

It might be useful to have a comment paragraph just above here that gives a brief overview of how this all works (but just a few sentences).

astrofrog · 2013-02-08T20:29:03Z

@iguananaut - I'm going to test this again, but it all looks good to me. I just added a comment in a place where mayb 2-3 sentences could be added to give a high-level overview of how the constants code works in general. But that's just for convenience for future developers, and shouldn't hold things back.

astrofrog · 2013-02-08T20:47:55Z

@iguananaut - just tested it again, and it's behaving perfectly now! I think this is ready to go.

astrofrog · 2013-02-08T20:49:57Z

@iguananaut - if you could include it in this PR, since it's somewhat related, could you fix the mistake in Quantity.__sub__: in the error message, it should say 'Subtraction', not 'Addition' (i.e. if you do G - 100 it will mention addition in the error message).

embray · 2013-02-09T00:15:48Z

Granted, addition and subtraction aren't really distinct operators, but okay :)

eteq · 2013-02-10T00:46:25Z

The latest version seems to have broken something - I can't get the build details to appear on travis, but it fails for me locally - there's a syntax error due to a parenthesis that doesn't seen to be in the right place on line 899 in core.py (I think that was supposed to be for line continuation, but it looks like it won't let you do the parenthesis when you change variable assignments together)

eteq · 2013-02-10T00:47:45Z

Also odd is that the previous build no longer seems to complete its scipy build in time, even though this worked before. This doesn't change anything about the scipy build, right?

astrofrog · 2013-02-10T10:58:45Z

@eteq - the scipy build could happen if the syntax error in core.py is causing lots of error messages, as it will make a lot of tests fails and the build slow down. So I think maybe just fixing the syntax error will make things work again.

eteq · 2013-02-11T06:03:37Z

See https://travis-ci.org/eteq/astropy/builds/4715685 for a build that's identical to this branch, but with the syntax error fixed. They all pass!

astrofrog · 2013-02-11T15:54:12Z

All other changes around here appear to be PEP8, but is this one? It seems to move the assignment inside a block?

That change was from an earlier commit (f8f3455) which changed the _registry from a list to a dict. It's moved inside the loop because a registry entry is created for each name a unit has. There was something that was made more efficient this way but I can't remember what exactly.

…e, and then uses e.si, e.gauss, e.esu (e.gauss and e.esu in this case are equivalent). e.cgs raises an exception. e still combines with other quantities using its SI value in Coulombs by default, though we could (and perhaps should) modify it to raise an exception by default if a specific system is not used.

…ned, currently just makes that constant an EM constant and requires it to specify a system if it's to be used as a quantity. Not fully working yet though, and the concept is not unique to EM constants either so the concept might be further refined and generalized.

… the astropy.constants module, reworking the imports so that the circularity between astropy.units and astropy.constants is overcome, and reworking the Constant class to have a registry of constants in different systems, among other differences.

…it formatting that was previously used in the docs

…nt defined in that system if it exists, else convert to a Quantity in the appropriate new units.

… avoid a potentially unnecessary slow unit conversion

…sample system

…nboundLocalError when using e in an expression more than once. Cleaned up no longer relevant comments and docstrings, and updated the documentation.

…t but it's a start) and fixed the mentioned error message for Quantity.__sub__

…ment expressions)

… and the like--if someone really wants those they can either ask for them or derive them themselves--I don't know how commonly these are used?)

embray · 2013-02-12T23:13:58Z

Had to rebase mostly due to #761. Anyways, going ahead and merging this.

Alternate constants API 2

Alternate constants API 2 Conflicts: astropy/units/format/generic.py

eteq · 2013-02-12T23:36:36Z

Hooray!

… should be using the value of c (in m/s) as a scaling factor, but its units shouldn't really come into it. This should have been changed to c.value and h.value as part of astropy#745.

Alternate constants API 2 Conflicts: astropy/units/format/generic.py

…sed an UnboundLocalError when using e in an expression more than once. Cleaned up no longer relevant comments and docstrings, and updated the documentation.

…ctor-rebase Alternate constants API 2 Conflicts: astropy/units/format/generic.py

astrofrog reviewed Feb 8, 2013
View reviewed changes

Comment thread astropy/constants/__init__.py

astrofrog Feb 8, 2013

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.

The docstring above needs to be updated as it refers to the old API.

astrofrog reviewed Feb 8, 2013
View reviewed changes

astrofrog reviewed Feb 11, 2013
View reviewed changes

astrofrog and others added 14 commits February 12, 2013 18:12

Fix failing test in Python 3.2

dce3f68

I think these should be the other way around.

02ed4ce

Got the constants module docstring table working using the default un…

0846ad2

…it formatting that was previously used in the docs

Override the si and cgs properties from Quantity to return the Consta…

f153bf7

…nt defined in that system if it exists, else convert to a Quantity in the appropriate new units.

Use short-circuiting instead of relying on the get() default value to…

2a48101

… avoid a potentially unnecessary slow unit conversion

Fix the physically incompatible units error to not display None as a …

c7d9b9f

…sample system

Fixed issues mentioned in astropy#745. Fixed the bug that caused an U…

ab2accf

…nboundLocalError when using e in an expression more than once. Cleaned up no longer relevant comments and docstrings, and updated the documentation.

Added the requested docstring on ConstantMeta (which still isn't grea…

3a73f2c

…t but it's a start) and fixed the mentioned error message for Quantity.__sub__

PEP8 because might as well while we're at it

1b98320

Fix syntax error (didn't realize parentheses aren't allowed in assign…

7a33203

…ment expressions)

Adds a couple more EM units (I didn't go crazy adding statV and abOhm…

68f574c

… and the like--if someone really wants those they can either ask for them or derive them themselves--I don't know how commonly these are used?)

embray added a commit that referenced this pull request Feb 12, 2013

Merge pull request #745 from iguananaut/constants-refactor-rebase

d36a357

Alternate constants API 2

embray merged commit d36a357 into astropy:master Feb 12, 2013

embray deleted the constants-refactor-rebase branch February 12, 2013 23:19

embray added a commit that referenced this pull request Feb 12, 2013

Merge pull request #745 from iguananaut/constants-refactor-rebase

02e724d

Alternate constants API 2 Conflicts: astropy/units/format/generic.py

keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013

Merge pull request astropy#745 from iguananaut/constants-refactor-rebase

2656c5b

Alternate constants API 2 Conflicts: astropy/units/format/generic.py

eteq mentioned this pull request Nov 29, 2015

Electric charge base unit in cgs should not be Coulomb #4354

Closed

embray mentioned this pull request Dec 2, 2015

Context manager for CGS E&M units? #4359

Open

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020

Merge pull request astropy/astropy#745 from iguananaut/constants-refa…

193ff06

…ctor-rebase Alternate constants API 2 Conflicts: astropy/units/format/generic.py

pllim mentioned this pull request Jan 13, 2026

Fix docstring for solar luminosity #19148

Closed

1 task

Conversation

embray commented Feb 7, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

astrofrog Feb 8, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

mdboom commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

adrn commented Feb 8, 2013

Uh oh!

embray commented Feb 8, 2013

Uh oh!

astrofrog Feb 8, 2013

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

astrofrog commented Feb 8, 2013

Uh oh!

embray commented Feb 9, 2013

Uh oh!

eteq commented Feb 10, 2013

Uh oh!

eteq commented Feb 10, 2013

Uh oh!

astrofrog commented Feb 10, 2013

Uh oh!

eteq commented Feb 11, 2013

Uh oh!

astrofrog Feb 11, 2013

Choose a reason for hiding this comment

Uh oh!

embray Feb 11, 2013

Choose a reason for hiding this comment

Uh oh!

embray commented Feb 12, 2013

Uh oh!

eteq commented Feb 12, 2013

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants