Support_nddata: support more options: keep, attribute_argument_mapping by MSeifert04 · Pull Request #5477 · astropy/astropy

MSeifert04 · 2016-11-16T19:21:39Z

When trying to apply the support_nddata decorator for some functions in ccdproc (astropy/ccdproc#415) I've noticed quite a few shortcomings:

It offers no option for attributes that should be copied from the input to the output. For example meta and unit can often simply be copied to the result (if repack was given). Therefore I included an keeps-argument.
Sometimes the first argument simply isn't data or the mask is simply not called mask. To ship around these problems I've included optional **kwargs to specify this mapping from attribute->function argument.
NDDataArray or other NDData subclasses might not support giving data as keyword-argument. This was fixed but it still affects affiliated packages that want to use repack and also support LTS.
If the returns contained more than one element but the function in fact returned just one it lead to an amusing (confusing) unpacking of the returned numpy.array. Therefore it checks if the return is a tuple before checking it's length and/or unpacking it.

Most of the changes just reduce the number of nested ifs or slightly modify the existing behaviour.

MSeifert04 · 2016-11-16T23:19:29Z

+           Must be ``None`` if ``repack=False``.
+
+        .. warning::
+           If the decorated function should work with `CCDData`, you *probably*


Reminder: Use double backticks for CCDData - we're not in ccdproc anymore.

astrofrog · 2016-11-29T09:09:42Z

-
-    If the user passes an NDData object *and* explicitly sets a keyword
-    argument that is one of the valid NDData properties, a warning is emitted
-    to inform the user that the explicitly specified value will take priority.


What happened to all the documentation above?

That's pretty much covered in the narrative docs. I'll see which parts should be noted in the Notes-section.

It looks like almost all of this has been moved either into the (fantastic) detailed parameter descriptions below, or is covered in the narrative documentation, but I think I see a couple things missing still:

One (important) thing that has been lost in the edits, though, is an explanation of what the data argument has to be: The first positional argument should be ``data`` and take a Numpy array. I know this has changed a bit -- data maybe a positional argument or a keyword argument named data (and that name can be overridden using the new attribute_argument_mapping) but I think an explicit mention of it in the notes would be helpful.

Having one example in this inline documentation still seems useful even though additional example are in the narrative documentation. I'm thinking the part from above that begins For example, ... and continues for a few paragraphs, in the Examples section of the docstring.

@MSeifert04 -- let me know if you want me to push those edits to your branch.

@astrofrog -- I believe all of the pieces of the deleted documentation are now either in back in the docstring or are covered in the narrative documentation.

astrofrog · 2016-11-29T09:10:37Z

+    -----
+    This is a slightly modified version of `~astropy.nddata.support_nddata`, so
+    for more hints checkout their documentation or have a look at the
+    ``ccdproc.core.py`` code.


Outdated note?

Oups, thanks 👍

astrofrog · 2016-11-29T09:10:54Z

-    if returns is None and repack:
-        raise ValueError('returns should be set if repack=True')
+    if (returns is not None or keeps is not None) and not repack:
+        raise ValueError('returns or keep should only be set if repack=True.')


astrofrog · 2016-11-29T09:12:44Z

+        _doc = '\n'.join(_tbl)
+        print(_doc)  # print to get the nice output.
+        # # End of Table creation.
+        """


The above should be removed

MSeifert04 · 2016-12-01T02:36:57Z

Most comments were about the documentation, are there any comments on the internals?

I would really like to get this into 1.3 ... is that realistic?

mwcraig · 2016-12-01T04:34:08Z

@MSeifert04 -- I'll take a look first thing in the morning. This is pretty short, so should still be possible to get in.

mwcraig

Thanks again for working on this!

I think there is a little bit of docstring rewrite that would be helpful and I have a couple questions inline. I can push a docstring fix you would like.

mwcraig · 2016-12-01T18:18:41Z

-
-    If the user passes an NDData object *and* explicitly sets a keyword
-    argument that is one of the valid NDData properties, a warning is emitted
-    to inform the user that the explicitly specified value will take priority.


It looks like almost all of this has been moved either into the (fantastic) detailed parameter descriptions below, or is covered in the narrative documentation, but I think I see a couple things missing still:

One (important) thing that has been lost in the edits, though, is an explanation of what the data argument has to be: The first positional argument should be ``data`` and take a Numpy array. I know this has changed a bit -- data maybe a positional argument or a keyword argument named data (and that name can be overridden using the new attribute_argument_mapping) but I think an explicit mention of it in the notes would be helpful.

Having one example in this inline documentation still seems useful even though additional example are in the narrative documentation. I'm thinking the part from above that begins For example, ... and continues for a few paragraphs, in the Examples section of the docstring.

@MSeifert04 -- let me know if you want me to push those edits to your branch.

mwcraig · 2016-12-01T18:30:19Z

-    If the user passes an NDData object *and* explicitly sets a keyword
-    argument that is one of the valid NDData properties, a warning is emitted
-    to inform the user that the explicitly specified value will take priority.
+# All supported properties except "data" which is mandatory!


properties except → properties are optional except?

mwcraig · 2016-12-01T18:32:07Z

-                    # We only need to do something if the property exists on the
-                    # NDData object
-                    if hasattr(data, prop):
+                for prop in islice(SUPPORTED_PROPERTIES, 1, None):


I don't think islice is incorrect here but is there a reason to not just do

for prop in SUPPORTED_PROPERTIES:

Because the data (first item in SUPPORTED_PROPERTIES) must be in the function signature so there is no need for any of these checks.

Ah, right, but why not SUPPORTED_PROPERTIES[1:]? Not a big deal (can be merged as is) just curious.

The [1:] creates a new list while islice(..., 1, None) creates an iterator. But both are equivalent if used in an for x in thing.

If you think [1:] is more readable I'll gladly change it. The islice is much better if the list is big but for a small list it won't matter either way.

Let's not worry about it given the large number of other things to be done before feature freeze.

mwcraig · 2016-12-01T19:11:21Z

+                        # If it's in the func_args it's trivial but if it was
+                        # in the func_kwargs we need to compare it to the
+                        # default.
+                        # FIXME: This obviously fails if the explicitly given


Just to make absolutely sure I understand the issue: it is that a keyword argument to the function will always have a value (this is what makes it a keyword), so you are trying to check whether the value is different than the default and only issue the warning if they are different.

But that case is indistinguishable from the case where the user has explicitly given a value that is the default.

In that case does an unnecessary warning generated or does the property from the NDData object get used instead? Looks to me like the latter, which seems more problematic.

Yes, it's the latter and it's somewhat problematic.

But there is no reliable way to tell if it was passed explicit or if it's just the default.

~~Let's not worry about it given the large number of other things to be done before feature freeze.~~ Wrong place for this comment.

mwcraig · 2016-12-01T19:11:51Z

+                        # violating the sphinx-documentation for that function.
+                        if (propmatch in func_args or
+                                (propmatch in func_kwargs and
+                                 not np.array_equal(kwargs[propmatch],


What happens for supported properties that are not numpy arrays (e.g. WCS)?

It works with WCS because it does an element-wise comparison of object-arrays then. But I've noticed some classes (fits.Header) when it doesn't work. I do have an idea how to make it work though. Will push a fix later.

I've changed this to use an identity comparison. That might still fail for interned data types like the integers from -5 to 255 but at least it works in more cases than before.

@mwcraig

Also uses explicitly passed in argument if it is not identical (same id) to the default. Before it compared equality instead of identity. Includes small documentation fixes suggested by @mwcraig.

MSeifert04 · 2016-12-01T21:48:05Z

I haven't been able to fix all problems. Even using an is comparisons can be really tricky in some cases:

>>> @support_nddata
... def func(data, wcs=5):
...     return wcs

>>> func(NDData(1, wcs=3), 5)  # should return 5 because it's explicitly given
3

>>> @support_nddata
... def func(data, wcs=[1,2,3]):
...     return wcs

>>> func(NDData(1, wcs=[1,2]), [1,2,3])  # correctly returns explicitly given [1,2,3]
[1,2,3]

@mwcraig Feel free to commit against this PR 👍

MSeifert04 · 2016-12-01T21:50:13Z

@mwcraig Thank you for your comments!

mwcraig · 2016-12-02T02:14:21Z

+                        # as well). In that case the default is
+                        # indistinguishable from an explicitly passed kwarg
+                        # and it won't notice that and use the attribute of the
+                        # NDData.


I think in practice this is not an issue because none of the supported properties can have a value like that. The exception might be units, though I'm having a little trouble imagining a real use case in which the data already has a unit, the function being called has a unit argument with a default unit, and the user tries to override that by specifying the unit that happens to be the default rather than using the unit attached to the data.

They shouldn't but these are loose recommendations for NDData. wcs and mask are completly free of restrictions and could be anything.

I would feel better if the comment stays just so it's clear that it could be a problem, even if it's unlikely. But maybe the word FIXME could be dropped because I don't have any idea how to fix it 😄

We can leave the comment in; although wcs and mask don't have much in the way of restrictions I'm not sure what wcs=5 would mean 😀. Probably might as well remove the FIXME.

I'm not sure what wcs=5 would mean

isn't it obvious? 😆

I realize the example wasn't that good but the same is True for None, False and True:

>>> @support_nddata ... def func(data, wcs=None): ... return wcs >>> func(NDData(1, wcs=WCS()), None) # should return None because it's explicitly given some_wcs_instance...

MSeifert04 · 2016-12-02T12:17:41Z

Just a heads-up: I cancelled the OSX build because they stalled the travis queue. The other tests passed.

MSeifert04 · 2016-12-02T18:44:32Z

@astrofrog Is your "request changes" review still valid? Otherwise I would like to get this into 1.3.

MSeifert04 · 2016-12-03T11:40:47Z

Am I right and this isn't being merged into 1.3?

mwcraig · 2016-12-03T16:19:36Z

It is still milestones at 1.3 so I think it will make it in. Work on it was complete yesterday; the hard cutoff for merging is when the 1.3.x branch is created. That is typically a couple days after feature freeze to allow a little time to catch up on merging things that were complete by freeze but which were not merged on the freeze date.

eteq · 2016-12-05T06:33:05Z

Did a quick final review myself, and I think this is all set. So I'm going to go ahead and merge to make sure this does indeed make 1.3 (as we haven't yet done the branching). Thanks @MSeifert04!

MSeifert04 changed the title ~~Support more options like keep and mapping to wrapped function names~~ Support_nddata: support more options: keep, attribute_argument_mapping Nov 16, 2016

pllim added nddata Enhancement labels Nov 16, 2016

mwcraig self-assigned this Nov 16, 2016

MSeifert04 commented Nov 16, 2016

View reviewed changes

astrofrog requested changes Nov 29, 2016

View reviewed changes

MSeifert04 added 2 commits November 29, 2016 18:51

support more options like keep and mapping to wrapped function names

feb0f6c

updated narrative docs and included feedback from PR comments

f9df883

mwcraig added this to the v1.3.0 milestone Dec 1, 2016

mwcraig requested changes Dec 1, 2016

View reviewed changes

allow numpy-arrays as parameter default value.

11535da

Also uses explicitly passed in argument if it is not identical (same id) to the default. Before it compared equality instead of identity. Includes small documentation fixes suggested by @mwcraig.

mwcraig reviewed Dec 2, 2016

View reviewed changes

Restore and clean up example in docstring

ebdb09a

mwcraig approved these changes Dec 2, 2016

View reviewed changes

eteq closed this Dec 5, 2016

eteq reopened this Dec 5, 2016

eteq merged commit dac5510 into astropy:master Dec 5, 2016

eteq mentioned this pull request Dec 5, 2016

Remove unnecessary headers and "warning" note in nddata decorator docs #5553

Merged

MSeifert04 deleted the support_nddata_more_options branch December 5, 2016 07:36

bsipocz mentioned this pull request Jan 25, 2017

Coverage testing is stopped since 1.2.2 release #5701

Closed

Conversation

MSeifert04 commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Dec 1, 2016

Uh oh!

mwcraig commented Dec 1, 2016

Uh oh!

mwcraig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwcraig Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented Dec 1, 2016

Uh oh!

mwcraig Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Dec 2, 2016

Uh oh!

MSeifert04 commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

mwcraig Dec 1, 2016 •

edited

Loading

MSeifert04 commented Dec 1, 2016 •

edited

Loading

mwcraig Dec 2, 2016 •

edited

Loading

MSeifert04 Dec 2, 2016 •

edited

Loading

MSeifert04 commented Dec 2, 2016 •

edited

Loading