@mwcraig wcs looks great, I had a quick look over the rest of it.

@mwcraig - regarding FlagCollection - I guess it depends if we want to assume anything about flags now we are moving to a more generic data container. It sounded like we agreed that flags doesn't even have to be a property in the base class?

Basically I think we had agreed that we should strip everything out of NDData apart from the property definitions, correct? That is, there should basically be no functionality in the core NDData (apart from using data.mask and data.unit if available).

That was my recollection too -- I'll update tonight or tomorrow.

Other than eventually supporting slicing, I would agree.

Finally got around to pulling out flags tonight.

I've left in slicing and the methods (like __array_prepare__) that allow an NDData object to be treated like a numpy array. That is primarily so that recovering a masked numpy array from a masked NDData object is easy. The properties like dtype, etc., make sense to keep with slicing.

An alternative is to moving slicing and those properties into NDDataArithmetic.

@mwcraig - are you also planning on simplifying the content of the uncertainty and mask properties? I think we had decided that we are going to strip these down to something like:

@property
def uncertainty(self):
    return self._uncertainty

@uncertainty.setter
def uncertainty(self, value):
    self._uncertainty = value

so that NDData is truly just defining the API but not really making any assumptions about checking, etc. For masking the only difference is that the getter would also check the mask property on the data.

Then for slicing I would simply make it that the __getitem__ will loop over the different properties, and for each one will try and slice them if they are not None (so then it's up to things like WCS to define how to be sliced). If any of them cannot be sliced, the operation fails.

For clarity, it might be nice to move the arithmetic classes to a separate arithmetic.py file.

@astrofrog --

For mask the line setting mask to a numpy array could certainly go. Are there reasons not to check the shape, though? I see that as something which may catch user errors more than limiting what can be done with the mask.

The bits about np.ma.nomask only need to stay if base NDData keeps its __array__ and __array_prepare__ methods. It would make sense to move those "math-y" aspects to arithmetic.py.

The bulk of uncertainty is handling the possibility that the uncertainty is specified with units. It could be vastly simplified by simply raising an error in the uncertainty setter if the value has a units property. I think the check for units is there because of the expectation that a user passing in data with units is also likely to be setting an uncertainty that has units, and if the user does then NDData should make sure the units are consistent.

Looking over the code that handles that I think it could be simplified and the restriction that the uncertainty be a NDUncertainty can be removed (or at least imposed only in the NDDataArithmetic subclass).

I'll update PR this evening, then we can continue the discussion from there.

@mwcraig - thanks for the comments. Just to be clear, what I'm suggesting here and in the draft APE that you saw is that we completely remove all uncertainty code from the base class. Of course, we could have a mix-in class that enables uncertainty stuff but in the default NDData class, this would not be present (according to the APE). We should discuss these points as part of the APE since the APE makes the implementation path very clear for the base class.

@mwcraig - regarding the shape of the mask, again it depends. For our SpectralCube package, we actually are not using arrays for the mask but lazily evaluated functions - for example we can do cube.mask = np.inf (or something similar at any rate) and it gets evaluated as needed. This is an example of why the base class should not even be assuming the mask is an array. One could also imagine having sub-classes where it might make sense to allow masks to be broadcast.

@astrofrog -- that makes sense...I keep forgetting that in the new arrangement NDData wouldn't be used directly. I'll make more extensive modifications, then, so people have something concrete to look at and try out if they want to see how the changes might affect them.

I'll include a subclass (maybe in arithmetic, maybe somewhere else) that acts like the old NDData to make the transition as easy as possible for anyone who hasn't been involved in this.

I don't know that NDData by itself couldn't be used directly. I think it can and should still be useful as a container for arrays and metadata (and uncertainties? I forget if that is still on). It's just that it would be more useful as a base class for more sepcific data.

@mwcraig - now that the APE has been accepted, can you finalize this PR to implement the decisions in the APE? On my side I'll finish up the implementation of the decorator stuff.

@astrofrog - Working on it, will see how far I get today.

@astrofrog -- mostly done, still need doc updates and an I/O mixin (though I suppose that could come later)

Any suggestions for this? [Am currently thinking "wait til later" is the best approach for now]

In [4]: class Foo(Quantity, NDDataBase):
   ...:     pass
   ...:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-bf917ed358f2> in <module>()
----> 1 class Foo(Quantity, NDDataBase):
      2     pass
      3

TypeError: Error when calling the metaclass bases
    metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

I think you need to define a metaclass for Foo that subclasses both InheritDocstrings and ABCMeta. I feel like there might be a way to avoid having to do this in the future, but for now that's what would be necessary. I know I've had to do things like that before when I've run into this error, but I forget the exact context.

@mwcraig - I can work on the IO mix-in on Friday since I'm familiar with that part. I'll review the rest of what's here tomorrow. Thanks!

@mwcraig - once that is fixed, an you also rename this issue to remove the WIP?

@astrofrog -- fyi, am making progress on I/O mixin, will push today.

@astrofrog -- my attempt at an i/o mixin has been pushed.

Some caveats/obvious limitations:

• It only implements a FITS reader/writer
• I may have used classmethods unnecessarily...I got fairly confused at one point.
• If you read from a FITS file then the metadata is kept as a FITS header. Probably that should be controllable by an optional argument, since I can image cases where either that behavior or one in which the metadata is converted to a dict with some cruff removed, like in Table, is desirable.

Longer term, I think there is some refactoring to be done to reduce duplicate code between this and io.fits.connect, which implements readers/writers for Tables but contains some stuff that is not Table-specific (that I copied and pasted here).

@mwcraig - thanks, I will do a PR to your branch to bring the I/O stuff in line with what Table does.

@mwcraig - see mwcraig#1 - however, unless you agree with my changes there, I think this may actually take some more discussion so it might make sense to remove the I/O commit from this PR (first let's see if you agree with mwcraig#1)

@mwcraig - if you disagree with the changes in my pull request, then in order to make sure we merge this PR today, please remove the latest IO changes and instead simply add a very simple IOMixin class:

class NDIOMixin(object):
    """
    Mixin class to connect NDData to the astropy input/output registry.

    This mixin adds two methods to its subclasses, ``read`` and ``write``.
    """

    @classmethod
    def read(cls, *args, **kwargs):
        """
        Read and parse gridded N-dimensional data and return as an
        NDData-derived object.

        This function provides the NDDataBase interface to the astropy unified
        I/O layer.  This allows easily reading a file in the supported data
        formats.
        """
        return io_registry.read(cls, *args, **kwargs)

    def write(self, *args, **kwargs):
        """
        Write a gridded N-dimensional data object out in specified format.

        This function provides the NDDataBase interface to the astropy unified
        I/O layer.  This allows easily writing a file in the supported data
        formats.
        """
        io_registry.write(self, *args, **kwargs)

and nothing else (no auto-registration).

For Astropy 1.0, affiliated packages will then be able to define and register their own readers/writers for their NDData subclasses if needed.

We can then postpone to later the decision of which built-in readers/writers to provide in the core, what their behavior should be, and whether NDData should inherit from IOMixin.

Sorry for the multiple messages - actually I would suggest that regardless of whether you agree with mwcraig#1 you follow the suggestion in my previous comment and remove the FITS reader/writer to keep this PR simple. Then later today I can open a PR with the changes I am suggesting but we can decide to defer to 1.1 if needed.

I agree with @astrofrog that if at all possible it would be best to bring in any FITS-specific stuff in a separate change.

@astrofrog -- just pushed the minimal version of the mixin you proposed, with no readers/writers registered. Agree with you and @embray that it makes sense to punt on this for now.

I'm fine with the rearrange in mcraig#1 but didn't merge because we went simple here. Please ping me if you do a PR and I''ll take a closer look.

@mwcraig - thanks for making the change back to a simpler I/O class.

If anyone else has any comments on this, please review this now, otherwise I will merge this later this evening (I will do a final review now).

These are my only comments - other than that this is good to go. Thanks for your work on this @mwcraig!

My pleasure; the comments are addressed.

@mwcraig - thanks! I will merge this tomorrow morning if there are no other objections. This implements APE7 so the API has already been discussed, and the implementation looks good to me.