Add SigmaClipped 8000 Stats class#17221
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
I focused on function signatures and type annotations. Since it's starting to pile up I'm going to keep it as that for this first round, but in short: I believe some confusion around the meaning of | None as an annotation may have slipped in from #16562, and I'd like to address that in a separate PR1, but I made a lot of comments here that boil down to "let's not propagate this mistake further".
Footnotes
|
@neutrinoceros Thanks for the review! The type annotations are due to me blindly copying the function signatures. I'm fixing them now. I'll also make the new class keyword-only. |
|
Do we need What's New? |
c6cb73f to
0af8761
Compare
|
@pllim I added a what's new entry. |
|
I'll review again in the morning. In the mean time, if you have a minute, it could be useful to rebase now that #17226 is merged, so I can checkout this branch and not stumble upon errors that I already fixed. Not a big deal if you can't though. Thanks ! |
b33b72c to
90212b9
Compare
|
@neutrinoceros Rebased. |
90212b9 to
d1631eb
Compare
|
I just pushed an update to ensure that the correct units are returned when using |
|
let's ask @mhvk |
There was a problem hiding this comment.
Couldn't help but notice that sigma=3 is the only value exercised in tests and docs. Also, it's always passed explicitly, which seems to indicate that you don't actually want a default value for it, but of course the same argument holds for SigmaClip, and it's a bit late to change that, so I would recommend having a test exercising not passing sigma explicitly.
astropy/stats/nanfunctions.py
Outdated
| if isinstance(array, Quantity): | ||
| return array.__array_wrap__(result) | ||
| if function == bottleneck.nanvar: | ||
| result <<= array.unit**2 |
There was a problem hiding this comment.
We know here that array is a Quantity, so can use the private method array._result_as_quantity(result, unit**2) - that would keep any subclass in place but ensure that, e.g., an Angle will drop down to a Quantity.
There was a problem hiding this comment.
Thanks, @mhvk! I've updated this to use _result_as_quantity and added a test for Angle input.
d1631eb to
d7fc389
Compare
d7fc389 to
28c602f
Compare
28c602f to
cfb67fc
Compare
There was a problem hiding this comment.
This looks good overall, just a small suggestion for the tests - though not important enough not to approve
cfb67fc to
1bf5dfc
Compare
1bf5dfc to
58efcac
Compare
|
The linktest failure is unrelated. |
Description
This PR adds a
SigmaClippedStatsclass that extends thesigma_clipped_statsfunction. The function currently calculates only mean, median, and std. But if you want only one of those, it still calculates all 3. The new class calculates only the statistics that you request. It also adds the following stats: var, mode, biweight_location, biweight_scale, and mad_std.