Skip to content

Fix bug in sigma clip mask - #3792

Merged
pllim merged 5 commits into
astropy:masterfrom
larrybradley:sigmaclip_mask
May 26, 2015
Merged

pllim merged 5 commits into
astropy:masterfrom
larrybradley:sigmaclip_mask

Conversation

@larrybradley

Copy link
Copy Markdown
Member

Recent changes to sigma_clip (#3595) made it possible for the mask to be scalar False in the returned masked array in the case where no values are clipped. The problem with that is the advertised method for getting the good values, good_only = filtered_data.data[~filtered_data.mask], does not work correctly with a scalar mask value. If the mask is scalar False then ~mask is scalar True, which is then translated to an index of simply 1.

This PR ensures the returned mask always has the same shape as data and uses np.ma.compressed in sigma_clipped_stats instead of filtered_data.data[~filtered_data.mask] to get the good data values.

@larrybradley

Copy link
Copy Markdown
Member Author

This PR also fixes a bug where sigma_lower and sigma_upper should have defaulted to None in sigma_clipped_stats.

Also, I took the opportunity to add the cenfunc, stdfunc, and axis keywords to sigma_clipped_stats (as suggested in #3595).

@pllim

pllim commented May 22, 2015

Copy link
Copy Markdown
Member

I don't see anything controversial with this. But perhaps the addition of new keywords is worth a mention in the change log. And addition to its documentation showing how to use the new keywords would not hurt.

@larrybradley

Copy link
Copy Markdown
Member Author

Thanks, @pllim. Done.

@embray embray added Bug stats Affects-dev PRs and issues that do not impact an existing Astropy release labels May 22, 2015
@embray embray added this to the v1.1.0 milestone May 22, 2015
@embray embray added Affects-release and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels May 22, 2015
@pllim

pllim commented May 22, 2015

Copy link
Copy Markdown
Member

👍 If no one objects or merge after Memorial Day, I will do it.

pllim added a commit that referenced this pull request May 26, 2015
@pllim
pllim merged commit 03fddbd into astropy:master May 26, 2015
@pllim

pllim commented May 26, 2015

Copy link
Copy Markdown
Member

@larrybradley , I merged it.

@larrybradley
larrybradley deleted the sigmaclip_mask branch June 3, 2015 18:53
saimn added a commit to saimn/astropy that referenced this pull request Mar 23, 2016
The axis argument was added in astropy#3792 and is passed correctly to sigma_clip, but
then mean, median and std did not use axis and were computed on the whole array.
This commit changes this to use axis to compute the statistics.

Also another minor annoyance is that, with Numpy 1.10, np.ma.median always
return a MaskedArray, even when axis is None. This was not the case in previous
version (and will probably be fixed in the future), and mean and std return
a scalar. So if it happens here, we take the first element of the array.
saimn added a commit to saimn/astropy that referenced this pull request Mar 23, 2016
The axis argument was added in astropy#3792 and is passed correctly to sigma_clip, but
then mean, median and std did not use axis and were computed on the whole array.
This commit changes this to use axis to compute the statistics.

Also another minor annoyance is that, with Numpy 1.10, np.ma.median always
return a MaskedArray, even when axis is None. This was not the case in previous
version (and will probably be fixed in the future), and mean and std return
a scalar. So if it happens here, we take the first element of the array.
saimn added a commit to saimn/astropy that referenced this pull request Apr 12, 2016
The axis argument was added in astropy#3792 and is passed correctly to sigma_clip, but
then mean, median and std did not use axis and were computed on the whole array.
This commit changes this to use axis to compute the statistics.

Also another minor annoyance is that, with Numpy 1.10, np.ma.median always
return a MaskedArray, even when axis is None. This was not the case in previous
version (and will probably be fixed in the future), and mean and std return
a scalar. So if it happens here, we take the first element of the array.
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.

3 participants