Took a very quick look, the approach looks good!

Let me know when you want a more thorough review. The decorator in ccdproc that changes function signatures is at https://github.com/astropy/ccdproc/blob/master/ccdproc/log_meta.py

It is not as broad as @embray's recent PR, I'm nearly positive.

Yup, looks good so far! I don't know if you want to add the issue of what the decorator returns (ie if you give it nddata-type object if it returns an nddata-type object), or to save that for later discussions.

@mwcraig - no need for a careful review yet, I plan to wait for #2849 to be merged which will take care of preserving the signature and docstring, then will add some tests and docs.

This looks good! However, it's currently restricted to functions that have only one positional argument (data). That will need to be relaxed to be useful for most functions in photutils.

@larrybradley - yes, good point. Can we impose the restriction that the data should be the first argument though? (cc @cdeil @bsipocz)

@astrofrog Yes, the restriction that data should be first is fine. I think everything in photutils/imageutils already does that, although it is sometimes called image instead of data (which can be easily changed).

ccdproc already sues the data-is-first-argument convention too.

@mwcraig @larrybradley @cdeil - the decorator is not more robust, well tested, and documented in the docstring. Could you try it out on some real-life cases to see if it works?

The function signature is not yet preserved because I need to wait for #2849 to be merged, then I'll update this code to use wraps from there.

@embray, the test here marked as xfail fails on Python 3.4 too - should I write the test differently? (I thought the built-in wraps would work since this isn't in IPython?)

Also, a general question - do we need a documentation page for this, or is the docstring enough?

@astrofrog I'm about to push a change to make it work on Python 3.4 as well, and then assuming all is good there will merge it. FYI I also am moving the new wraps out of astropy.utils.compat and into astropy.utils.misc (you should just use from astropy.utils import wraps). Then you can remove the xfail on that test.

Actually better still, you might just remove that test entirely. The new wraps is already tested on its own, so there's no need to test the function signature for every decorator it's used with.

@embray - I'll leave the test for now just to make sure that it all works fine, and I can always remove the test after that.

@astrofrog It's working with photutils functions. Do you know why the warning message is repeated?

WARNING: The following attributes were set on the data object, but will be ignored by the function: unit, wcs [astropy.nddata.decorators]
WARNING:astropy:The following attributes were set on the data object, but will be ignored by the function: unit, wcs

Users that have an NDData object as input probably want to have NDData as output in many use cases. Assuming the example from the docstring:

@expand_nddata_args
def downsample(data, wcs)

it's a bit weird that expanding the NDData on input is implicit, but I have to explicitly reassemble the output into an NDData:

nddata = NDData(data, wcs)
data_out, wcs_out = downsample(nddata)
nddata_out = NDData(data_out, wcs_out)

Even worse if I have an NDData with some property that the function doesn't support ... I have to write:

nddata = NDData(data, wcs, mask)
data_out, wcs_out = downsample(nddata.data, nddata.wcs)
mask_out, _ = downsample(nddata.mask, None)
nddata_out = NDData(data_out, wcs_out, mask_out)

What if I write a library function that calls downsample as one step and I don't know what properties the NDData input has set ... it's just hard to process an object like NDData that has a handful of optional properties in general-purpose functions correctly.

Since I wasn't able to participate in the telcon, let me just say once more that I'd prefer a more explicit API, where downsample for NDData is handled in a separate downsample_nddata function that takes NDData as input and returns NDData as output instead of leaving half of the NDData member processing to the end user.

@cdeil - before we continue the discussion further, I should just mention that there really are two kinds of functions here:

• Functions that return something that isn't or can't be interpreted as an NDData, for example aperture_photometry which returns a Table.
• Functions that return something that could be an NDData.

For the first category of functions I think there really is no reason not to use the decorator proposed in this PR - there is no point in duplicating the API. Do you have any objections to that?

For the second, here's a possible solution. The issue with duplicating the API is the repetition of a lot of boilerplate code, for example it would be good to warn the user if the uncertainties can't be used, etc. So we could basically have a slightly different decorator that rather than modifying the original function is used to generate an NDData-friendly wrapper:

downsample_nddata = wrap_nddata(downsample, ('data', 'wcs'))

where the ('data', 'wcs') tells the wrapper what kind of outputs to expect. The wrapper would create a new function that then takes care of re-assembling the NDData object. Would this be an acceptable solution @cdeil? If so, I can try and implement it here (and I think some of the code can be shared by both decorators/wrappers).

(Edit: in the second idea, if the functions conform to the numpydoc format, we can even edit the docstring on-the-fly to reflect the fact the input should be NDData and the return is NDData.)

I should admit that I think NDData itself is a bad idea.
A class that has a handful of pre-defined optional data members and provides almost no functionality?
It's just not useful and has costs (API complexity, more tests and docs) if it needs to be supported in other functions.

The only reason it works for CCDData is because it was made with that application in mind.
Try making SpectralCube an NDData sub-class and you'll see that it's not useful.

So far NDData has been there, but I could simply ignore it. Now there's a proposal that NDData should be supported (see astropy/imageutils#13 for imageutils, but probably the same proposals to support NDData will be made for reproject, photutils, astropy.convolution, astropy.modeling, ...) which means extra effort.

So my preference would be to get rid of NDData and leave it to each application domain (ccdproc, gammapy, spectral-cube, ..) to define their data container classes and implement operations like downsample, ... with the correct semantics for mask, uncertainty, ... for their application.

If we keep NDData and this decorator (which seems to be the case since I'm the only one that thinks NDData as a base class is an overall bad idea), yes, I'd much prefer to keep the basic function downsample simple and independent of NDData and have a separate function like
downsample_nddata that could be created via a decorator like wrap_nddata or hand-written if that doesn't work well for a given function for some reason.

I need to think about this a bit more, but if we go back to basics, I think the idea was really to provide a simple data container than was a generalized version of an HDU. At the base level we wanted to have a data container that could be format-agnostic and could be used to represent an n-dimensional array with some meta-data. But of course, once you start to interpret objects as e.g. a spectrum, IFU, image, you want to treat them differently.

So maybe the mistake is to try and write functions that can work with any NDData. No function should really accept any NDData objects because how you treat them depends on the application. For example, imageutils should really define its own NDData subclass, Image, and only accept that (and subclasses of that). This allows you to then have control over the representation of the data.

CCDImage could then inherit from Image and could use imageutils functions, and it would also define its own functions that can only operate on CCDImage.

You may ask what the point of even inheriting from NDData is in the first place, and I think the answer is that it enforces a common API for accessing data and meta-data, and it provides the I/O functionality.

I just found out about GrokTheDocs, if you click the link you see some info about which docs are read by the users. Table is the most popular data structure (by a small margin), NDData is least popular (with a large margin). :-)

Also all the work it's incorporated for handling masked data.

Maybe something we might end up doing is making NDData an abstract base class. You would never use it directly, but you can use it as a subclass for objects that really do represent something real such as an image or spectrum. I think it contains a lot of useful machinery that can and should be reused, but maybe the current lack of useful subclasses (at least, in the Astropy core) is what makes its use non-obvious.

It's no surprise that the docs for NDData aren't "popular" because, as @cdeil says, it doesn't really give you anything by itself. But I think it's going to be important for Astropy-compatible data containers to have some common-basis. And if there's any functionality in it that gets in the way of implementing more application-specific data containers on top of it then that functionality should be removed or moved to a more appropriate subclass (if it's still used somewhere else).

And agreed ... we should do 1.0 deadlines more often to get stuff done!
:-)

@cdeil - there is no real case in the core package but the decorator currently preserves the docstring of the decorated function, so we can start using it seamlessly in affiliated packages and then try and come up with something smarter for the docstring? Or come up with a docstring convention then?

+1 to merge this tomorrow ... and when the first real example exists in the core or photutils or ccdproc add a link to it from these docs.

@mhvk - I think indeed the approach you are suggesting could be considered an extension of what's here, so I think we are fine with deferring that for now. I added a note about this being experimental.

This can be merged once Travis and AppVeyor pass

Given that this is in line with APE7 which was approved, and that there have been no objections, I will go ahead and merge this. We can still tweak this during the beta-testing phase if needed. It would be great if affiliated package developers could try this out (@cdeil @mwcraig - worth trying out in photutils and ccdproc?)