I'll have to look at this after 0.2.1 is out. But I like where this is going....

+1,000,000 in principle. I'll have to play with this a bit, and it does seem to break every test configuration on Travis so there's probably some glitch here...

@mdboom - the tests are failing because astropy.constants is broken. This is very much an incomplete pull request at this stage, so any help is welcome :-)

Ah, very nice -- thanks @astrofrog !

@astrofrog before we proceed - Wasn't the idea that Quantity would carry uncertainties? Does that still work with being a subclass of ndarray?

I think this may come useful as a base class for models._Parameter class. This will make parameters usable in astrophysical models as it already handles units.

@nden 👍 to the idea of letting Parameters in models be Quantities!

One concern about this, though: this makes tab-completion very busy on Quantity, because it now picks up all the numpy methods. That's not necessarily a big deal - and #837 (which I think is a very good thing) is even more complex in that respect. But it's definitely something to keep in mind.

The other danger is that this now ties us to the numpy API permanently. I thought the whole points of this is to encourage q.value + somearray over just q + somearray, and doesn't this break that? Or at least, it allows people to break it, which means we have to support it? (I may be misunderstanding, though)

@eteq - the plan was not to allow q + somearray, as I think we can still control what kind of arrays one can combine quantities with. The point was to ensure that array * quantity returns a quantity, which is not the case at the moment. I'm not saying this is going to be easy, but it's worth the try, as it's also pretty cool to be able to do things like q.mean() and get back a quantity.

@wkerzendorf - the decision was to not have uncertainties in quantities (that's what NDData is for).

Except that Constants have uncertainties, they just aren't carried through when used in operations with other Quantities :/

re: uncertainties and constants: @iguananaut raises a good point here... and that would be a useful feature. I think the primary motivation for keeping uncertainties out of Quantity is that it makes a lot of things more complicated. Which is a wise choice, in my view, because otherwise we'd never have gotten it working. So the question is: once we have uncertainties propagating in NDData (and not before), should we consider making Constants NDData subclasses? At the same time we would want to make Quantity and NDData play nice with each other. That would allow work to progress on all fronts instead of having to worry about interactions between them.

@astrofrog - Ah, I see, yeah then my second issue is maybe not as big a deal. And would q.mean() return a Quantity object, or a float/0d ndarray? I would hope for the former.

I think we need to have a scalar class that carries errors along and i think nddata won't cut it there. The uncertainties in constants that @iguananaut mentioned are only one example: the fitting process for parameter models should be able to store it's calculated uncertainties with the parameters, etc. . Finally, I think adding uncertainties to Quantity will not encroach on NDData's turf as NDData is a much more complex beast (wcs, masks, flags, meta, ....) which will not be added to Quantity.

So in my opinion, the uncertainties-problem should be solved on a Quantity-level (and then trickle down the chain to nddata).

@eteq - q.mean() returns a Quantity (unfortunately you can't easily test it right now since astropy.constants is broken (but it works if you take Quantity out of Astropy).

I think that anyone can feel free to try and develop a framework for uncertainties, and we can then decide whether to attach it to Quantity, NDData, etc. But actually the more time goes on, the less important that feature seems to me, and I think it's actually going to be virtually impossible to implement. There's a lot more useful functionality that we need to develop (specutils, photutils, WCSAxes, etc.) that this is not a high priority (at least for me).

We also need to be a little bit careful with thinks like .prod and .cumprod that should inadvertently affect the units of quantity (in the latter case one cannot even construct a quantity object that correctly represents). These are the two immediate functions that came to mind, but there maybe others.

@astrofrog - ok, that sounds good - as @wkerzendorf says, we probably want tests to ensure ufuncs like prod and var and the like return the correct units, but I imagine we can just thinly override those. We'll just have to be careful in case ndarray adds more builtin ufuncs down the road that have weird units.

re: uncertainties: In my mind, a lack of uncertainties is one of the main things blocking implementation of things like specutils. Uncertainties are so central to proper science with such tools that they are nearly useless without them.

That said, @astrofrog has a very good point that we should develop the framework first, and later decide whether or not it ends up in Quantity. The more pressing need is in NDData, so we should work on it there first.

I have a feeling that eventually these two data structures will merge together, but in the meantime I'm happy to hack on them from different directions until they meet up in the middle.

@iguananaut - do you have any ideas of how astropy.constants should be fixed to get this to work? Will it require a major overhaul?

I'm trying to get a pyfits release out the door, but I'll start looking at this once that's done. Can you give me some idea of what the problem is with constants? Does it have something to do with the metaclass?

@iguananaut - yes, it's to do with the metaclass and __new__, and metaclasses are (for now) above my pay grade...

@iguananaut - I was wondering whether you could take a look at this, since I'm not sure I understand fully how the Constant meta-class works. I've rebased the branch on the latest master as there were some conflicts.

Taking a look now. No promises on what I'll find :)

@iguananaut - thanks!

Thanks to @iguananaut the test now pass, so anyone interested can try out this branch. There is however the issue that @wkerzendorf raised which is that many ufuncs are going to be returning results that are wrong in terms of the units. For instance, all the following are wrong:

In [10]: q = np.array([1,2,3]) * u.m

In [11]: q.mean()
Out[11]: 2.0  # should have units

In [12]: np.median(q)
Out[12]: 2.0  # should have units

In [14]: q.std()
Out[14]: 0.81649658092772603  # should have units

In [13]: q.cumprod()
Out[13]: <Quantity [1 2 6] m>  # each value should have different units

I'm not sure how we could best deal with things like cumprod. Ideally we would be able to disable all ufuncs and re-implement them with proper unit support.

@iguananaut - before you fixed this branch, the Quantity object did return a Quantity object for mean and std, but I noticed you removed __array_wrap__. Is there any way to get this to work again?

There's another thing worth thinking about - would it simplify things to have a separate class for Quantity and QuantityArray, only the latter of which would inherit from nddata? The user never accesses these classes directly anyway, so it'd be transparent to them. What do people think about this idea?

I've been trying to improve the test coverage and as part of this uncovered and fixed a few bugs. A couple did not have a trivial fix, so I've marked them as xfail and opened a new issue (#1273) since these bugs are not critical (they only occur for in-place use of specific functions/methods).

This is now ready for a final review! I will send an email to the dev list.

@iguananaut - can you confirm this works on Windows?

Well done! I just tried it out, looks very nice.

Thanks to everyone who contributed to this!

I got one relevant test failure on Windows with Numpy 1.7:

astropy\table\tests\test_table.py:708: AssertionError
___________ TestArrayConversion.test_byte_type_view_field_changes ___________

self = <astropy.units.tests.test_quantity_array_methods.TestArrayConversion object at 0x0A3FCEB0>

    def test_byte_type_view_field_changes(self):
        q1 = np.array([1, 2, 3]) * u.m / u.km
        q2 = q1.byteswap()
        assert q2.unit == q1.unit
        assert all(q2.value == q1.value.byteswap())
        q2 = q1.astype(np.float64)
        assert all(q2 == q1)
        assert q2.dtype == np.float64
        q2 = q1.view(np.ndarray)
        assert not hasattr(q2, 'unit')
        q2a = q1.getfield(np.int32, offset=0)
>       q2b = q1.byteswap().getfield(np.int32, offset=4)
E       ValueError: Need 0 <= offset <= 0 for requested type but received offset = 4

astropy\units\tests\test_quantity_array_methods.py:336: ValueError

Seems more an error in the test though. I'm not sure what that means or why the test is using getfield there.

Oh I think I see the problem. This is a 32-bit build of Python so the default dtype on that array is int32, not int64, so taking offset=4 doesn't make any sense. In order to be agnostic this test should explicitly write q1 = np.array([1, 2, 3], dtype=np.int64) at the beginning.

@iguananaut - do all the test pass now?

Any remaining comments, or shall we merge this later today?

@mdboom @iguananaut - does this look ready to merge from your perspective?

Yes, as far as I'm concerned.

Ok, so @iguananaut - could you check that everything passes on Windows, and merge this if so? Thanks!

Oh, I thought I updated you on this days ago, but I just realized I had a tab open where I started to do so, and then must have gotten distracted. Yes, all the (relevant) tests pass on Windows now.