Possible implementation of constants as quantities - #529

The approach is definitely very promising!

Is it necessary that the actual definitions live in units, though? I find it rather confusing that the definition of all the constants is now not actually in anything like constants. Would it be possible to instead do something like moving your astropy.units._constants_* modules into a special astropy._constants package (e.g. astropy._constants.cgs, astropy._constants.si, etc)? As long as astropy/_constants/__init__.py does not import the other packages, I think you won't hit circular dependencies. Then you can add a module astropy/constants.py that just imports the relevant parts from astropy._constants.

I think you can preserve basically all the code with a scheme, but give it a slightly cleaner file/package organization.

@eteq - ah, good point, I hadn't thought of doing it that way, I agree it would be less confusing to have a hidden _constants sub-package. Let me update that.

@eteq - I've implemented your suggestion. I think it helped clean things up conceptually. I still need to fix things like docstrings, docs, tests, etc. but I'll wait until I know whether people are happy going this way.

The tests are passing again.

@adrn @mdboom - what do you think of this approach?

This looks like a good approach. One minor suggestion -- it's probably entirely obvious to us, but to avoid confusion for those coming fresh to this it might be helpful to add the following comment to all of the mostly empty modules in astropy.constants:

# The constants in this module are defined in `astropy/_constants/si.py` in order to avoid circular dependencies.

I see a possibility for user confusion here:

In [1]: from astropy.constants import si

In [3]: si.G
Out[3]: 6.67384e-11

In [4]: type(si.G)
Out[4]: astropy._constants.definition.ConstantDefinition

In [6]: from astropy import constants

In [7]: constants.G
Out[7]: <Quantity 6.67384e-11 m3 / (kg s2)>

In [8]: type(constants.G)
Out[8]: astropy.constants.constant.Constant

Maybe we should override the ConstantDefinition objects in astropy.constants.si with Constant (i.e. Quantity) objects? Indeed, adding:

si.__dict__[nm] = c

to the bottom of the conversion loop seems to work.

Also, it seems there is no automatic way to get Constant objects for cgs constants (which aren't normally in the top-level astropy.constants namespace). Using the above technique, it should be possible to do astropy.constants.cgs.G and get a Constant object.

@mdboom - thanks for reviewing this. I will think about the issue you raised (and will add the notes you suggested).

In the mean time, how should we deal with si->cgs conversions for EM constants? (this is more of a question to do with Quantity than Units):

In [4]: e.cgs
ERROR: UnitsException: 'C / (Fr)' is not dimensionless [astropy.units.core]
---------------------------------------------------------------------------
UnitsException                            Traceback (most recent call last)
<ipython-input-4-ebc8d90fd7bd> in <module>()
----> 1 e.cgs

/Users/tom/Library/Python/2.7/lib/python/site-packages/astropy/units/quantity.pyc in cgs(self)
    171             if si_quantity.unit in _cgs._cgs_bases.keys():
    172                 # Don't have to worry about power here because if it has a power, it's a CompositeUnit
--> 173                 scale = (si_quantity.unit / _cgs._cgs_bases[si_quantity.unit]).dimensionless_constant()
    174                 cgs_quantity_value *= scale
    175 

/Users/tom/Library/Python/2.7/lib/python/site-packages/astropy/units/core.pyc in dimensionless_constant(self)
    987         if len(x.bases):
    988             raise UnitsException(
--> 989                 "'{0}' is not dimensionless".format(self.to_string()))
    990         return c
    991 

UnitsException: 'C / (Fr)' is not dimensionless

should we hard-code the cgs and si methods for such constants? (this is a question for @adrn too)

@mdboom - I'm going to reply to your comment on user confusion on the list, since this is something we were discussing there before, and the choice of having constants.si contain non-quantity objects was deliberate (but I'm not saying we should stick with it)

Maybe it's just me, but I guess I'm not clear on the need for the separate ConstantDefinition and Constant classes. I understand the former is more in the spirit of the original implementation, but is there anything that really depends on the old implementation that has float as its base? I also liked the informative __repr__ of the old class I wonder why it's necessary to remove it. Obviously if combine a constant with some other quantity you lose any information about the constant, but when it's just by itself why not keep the more detailed __repr__?

@iguananaut - first, regarding the repr, we can add it back, but I wanted to make sure the structure of the code was ok first. Regarding ConstantDefinition, we need to have the constants in a class because we want to keep all the information about the origin, units, and description in the same place, and also astropy.units uses them as floats to initialize some units, so it makes sense to have them be a sub-class of float. But of course we can't have them be a Constant since that relies on Quantity, which relies on Unit (hence the issue of circular dependency). What do you think?

Ah, okay. I didn't realize Constants were used to define some of the Units, but I guess that should have been obvious. Hmm--I feel like there should be a way around that. But this works fine for now so carry on :) Might be worth adding a note about that in the code--I know there was already a comment about circular dependencies, but I didn't get the full cycle of Constants are Quantities which have Units many of which are defined in terms of Constants.

Inception!

Just for info, I will have to wait until the weekend to implement the suggestions from here and the mailing list. But feel free to play around with the functionality in the mean time, to see if you can break it.

Sounds good, @astrofrog, and thanks for working this out!

(Also, I agree with @iguananaut that it'd be nice to get the slightly more informative __repr__ from the old Constant into this new Constant.)

Sorry, just catching up now...

@astrofrog This idea looks good to me and thanks for figuring this out!

Whoops sent that too soon -- I still think we should change all error to uncertainty!

I also don't really like origin, and in my implementation changed it to source -- but I'm not sold on that either.

@adrn - ok, will fix that.

@adrn - I changed error to uncertainty, and origin to reference.

@eteq and @iguananaut - I reinstated the __str__ and __repr__:

Out[4]: <Constant: name='Speed of light in vacuum' value=29979245800.0 error=0.0 units='cm / (s)' reference='CODATA 2010'>

In [5]: print cgs.c
  Name   = Speed of light in vacuum
  Value  = 29979245800.0
  Error  = 0.0
  Units = cm / (s)
  Reference = CODATA 2010

@mdboom (and everyone else): I have now moved all constants into cgs and si, and the floating point values are no longer available by value, only as Constant objects.

@mdboom and @iguananaut - I've added notes about the circular dependencies in all the relevant source files.

Just noticed there is no documentation page for constants. I will open a separate issue for that (and assign it to myself)

Shall we go ahead and review this for merging, and then if people have improved implementations later they can open a new PR? Has anyone had a chance to play with this?

This seems to address all of my concerns.

Looks good to me as well

I will merge this later today if there are no objections.