Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added sum combiner (#500) #508

Merged
merged 15 commits into from Jul 18, 2017
Merged

Added sum combiner (#500) #508

merged 15 commits into from Jul 18, 2017

Conversation

hletrd
Copy link
Contributor

@hletrd hletrd commented Jun 12, 2017

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?
  • 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.


@hletrd
Copy link
Contributor Author

hletrd commented Jun 13, 2017

Appveyor build fails on Python 3.6 because it fails to build Scipy. (Appveyor build and test on Python 2.7 succeed)

@hletrd hletrd mentioned this pull request Jun 13, 2017
Copy link
Contributor

@MSeifert04 MSeifert04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks really good, I only had a few minor comments.

However I think it should also include a test that tests for the correct uncertainty especially with some combinations of masked and unmasked pixels. Like:

  • none masked except one
  • none masked (already present but uncertainty isn't checked)
  • all masked (already present but uncertainty isn't checked)
  • all masked except one

CHANGES.rst Outdated
@@ -26,6 +26,8 @@ New Features

- ``combine`` now accepts ``numpy.ndarray`` as the input ``img_list``. [#493, #503]

- Added ```sum``` option in method for ```combime```. [#500]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also include the PR number [#500, #508]?

A `~ccdproc.CCDData` object is returned with the data property
set to the sum of the arrays. If the data was masked or any
data have been rejected, those pixels will not be included in the
sum. A mask will be returned, and if a pixel has been
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include a sentence that if a re-scaled average sum is needed one should do average_combine and multiply it with the number of images combined.

# set up the deviation
uncertainty = uncertainty_func(self.data_arr, axis=0)
# Divide uncertainty by the number of pixel (#309)
# Commented out because it cancels the effect of 'Multiply uncertainty by square root of the number of images'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be another line split. Even though 80 is just a "soft limit" I see no reason to make it much longer here.

@hletrd
Copy link
Contributor Author

hletrd commented Jun 25, 2017

  • Uncertainty test for partially masked image added in 2a7e2e6
  • Modified comments in 759e899

@hletrd
Copy link
Contributor Author

hletrd commented Jun 25, 2017

AppVeyor build only fails on Python 3.6 because it fails to build astroscrappy.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Jun 25, 2017

There seem to be 3 trailing whitespaces:

ccdproc/combiner.py:462:78: W291 trailing whitespace
        Because sum_combine returns 'pure sum' with masked pixels ignored, if
                                                                             ^
    Trailing whitespace is superfluous.


ccdproc/combiner.py:463:70: W291 trailing whitespace
        re-scaled sum is needed, average_combine have to be used with
                                                                     ^
    Trailing whitespace is superfluous.



ccdproc/combiner.py:507:77: W291 trailing whitespace
        # Commented out because it cancels the effect of 'Divide uncertainty
                                                                            ^
    Trailing whitespace is superfluous.

If these are removed that should fix the Travis build.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Jun 28, 2017

I'm still a bit confused about uncertainty for sum_combine:

>>> from ccdproc import CCDData, Combiner
>>> import numpy as np
>>> from astropy.nddata import StdDevUncertainty

>>> ccd1 = CCDData(np.arange(10), unit='', uncertainty=StdDevUncertainty(np.ones(10)))
>>> ccd2 = CCDData(np.ones(10), unit='', uncertainty=StdDevUncertainty(np.ones(10)))

>>> Combiner([ccd1, ccd2]).average_combine().uncertainty
StdDevUncertainty([ 0.35355339,  0.        ,  0.35355339,  0.70710678,
                    1.06066017,  1.41421356,  1.76776695,  2.12132034,
                    2.47487373,  2.82842712])

>>> Combiner([ccd1, ccd2]).sum_combine().uncertainty
StdDevUncertainty([ 0.5,  0. ,  0.5,  1. ,  1.5,  2. ,  2.5,  3. ,  3.5,
                    4. ])

It seems somewhat strange that the sum uncertainty isn't a multiplicative of the average combine. In case there is no masked pixel shouldn't it be the uncertainty from average-combine times the number of "combined" pixels?

@hletrd
Copy link
Contributor Author

hletrd commented Jun 30, 2017

I was wrong. Changed in 25c4e68.

@crawfordsm crawfordsm merged commit ecd90cb into astropy:master Jul 18, 2017
@crawfordsm
Copy link
Member

Thanks @hletrd !

@MSeifert04 MSeifert04 added this to the 1.3 milestone Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants