Skip to content

[No-MERGE] modified "support_nddata" decorator. - #415

Closed
MSeifert04 wants to merge 5 commits into
astropy:masterfrom
MSeifert04:median_filter
Closed

[No-MERGE] modified "support_nddata" decorator.#415
MSeifert04 wants to merge 5 commits into
astropy:masterfrom
MSeifert04:median_filter

Conversation

@MSeifert04

@MSeifert04 MSeifert04 commented Nov 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Please have a look at the following list and replace the "[ ]" with a "[x]" if
the answer to this question is yes.

  • For new contributors: Did you add yourself to the "Authors.rst" file?

For documentation changes:

  • For documentation changes: Does your commit message include a "[skip ci]"?
    Note that it should not if you changed any examples!

For bugfixes:

  • Did you add an entry to the "Changes.rst" file?
  • Did you add a regression test?
  • Does the commit message include a "Fixes #issue_number" (replace "issue_number").
  • Does this PR add, rename, move or remove any existing functions or parameters?

For new functionality:

  • Did you add an entry to the "Changes.rst" file?
  • Did you include a meaningful docstring with Parameters, Returns and Examples? Note: No examples!
  • Does the commit message include a "Fixes #issue_number" (replace "issue_number").
  • Did you include tests for the new functionality?
  • Does this PR add, rename, move or remove any existing functions or parameters?

Please note that the last point is not a requirement. It is meant as a check if
the pull request potentially breaks backwards-compatibility.

Fixes #371

TODO:

  • More wrapped functions?
  • Make a PR to astropy with the changed support_nddata decorator?

I've copied the support_nddata decorator from astropy.nddata to make it more general (specify attributes to keep, specify attribute->argument_name mapping.

Also included is a median_filter function that simply wraps scipy.ndimage.median_filter to test the decorator.

@MSeifert04 MSeifert04 changed the title [WIP] median_filter and patched "support_nddata" decorator. [WIP] median_filter and modified "support_nddata" decorator. Nov 2, 2016
@MSeifert04 MSeifert04 changed the title [WIP] median_filter and modified "support_nddata" decorator. median_filter and modified "support_nddata" decorator. Nov 2, 2016
@MSeifert04

MSeifert04 commented Nov 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think this is mostly ready now. In retrospect I don't know if it was a good idea to use a modified support_nddata. Originally I just wanted it to allow copying attributes from the input to the output but I found so many possible bugs/pitfalls that I think I completly overhauled it.

A wrapper function like:

def median_filter(data, *args, **kwargs):
    """See `scipy.ndimage.median_filter` for arguments. Enjoy!"""
    if isinstance(data, CCDData):
        out_kwargs = {'meta': data.meta.copy(), 'unit': data.unit}
        result = ndimage.median_filter(data.data, *args, **kwargs)
        return CCDData(result, **out_kwargs)
    else:
        return ndimage.median_filter(data, *args, **kwargs)

would have been much easier... 😞

@MSeifert04
refactored some code parts and adressed a really tricky part
0a9a413 
regarding default value, explicitly and implicitly given problem
@mwcraig

mwcraig commented Nov 15, 2016

Copy link
Copy Markdown
Member

Would it make sense to move the support_nddata changes back to astropy rather than maintaining a separate version here? I think there would be time to get it in for astropy 1.3. Detailed comments coming in a moment.

@mwcraig

mwcraig commented Nov 15, 2016

Copy link
Copy Markdown
Member

Just took a longer look. How about using your wrapper function idea here to get the median_filter in the next ccdproc relationship , and doing a PR on astropy to update @support_nddata (an idea which has seen little use in practice I think).

@MSeifert04 MSeifert04 added this to the someday milestone Nov 16, 2016
This was referenced Nov 16, 2016
Merged
Merged
@MSeifert04 MSeifert04 changed the title median_filter and modified "support_nddata" decorator. [No-MERGE] modified "support_nddata" decorator. Nov 18, 2016
@MSeifert04 MSeifert04 closed this Nov 23, 2016
@MSeifert04
MSeifert04 deleted the median_filter branch November 23, 2016 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ccdproc enhancement

Projects

None yet

Milestone

someday

Development

Successfully merging this pull request may close these issues.

Adding 'median' function

2 participants

@MSeifert04 @mwcraig