Several changes to sigma clipping functions - #3595

@embray Any suggestions about deprecation warnings?

I have a few suggestions on that, yeah. If you think they'll be necessary (they probably should be, since I know there are people actively using sigma_clipping).

Unfortunately if you do want to be able to support old arguments what you're probably going to have to do is change the argument signature for the function to accept (*args, **kwargs) instead of explicitly named arguments.

However, this is made somewhat easier by Python 3's new Signature handling. We have a version of this that is backported to work in Python 2 included in Astropy. You can read about the Signature spec here:

https://docs.python.org/3/library/inspect.html#introspecting-callables-with-the-signature-object

and the backported version in Astropy is found here:

https://github.com/astropy/astropy/blob/master/astropy/utils/compat/funcsigs.py

What you'll probably want to do create two Signature objects: One that represents the current function signature defined in your PR. You would then assign sigma_clipping.__signature__ = Signature...). The second Signature would support both the new arguments and the old arguments. You would use this one in the body of the sigma_clipping function itself to parse the (*args, **kwargs) with the bind method. Then you would unfortunately have to write some custom code to manually check if the user passed in any deprecated keyword arguments, and call warnings.warn(..., AstropyDeprecationWarning) for them. You would also have to make sure duplicates somehow aren't passed in (such as sig= and sigma= simultaneously).

Does that make any sense?

This reminds me I've wanted for a while to add a decorator specifically for this purpose...

@embray This is ready for review.

Needs a changelog entry briefly describing the API changes.

@embray I've updated the changelog and rebased.

👍 Is there an appropriate assignee to sign off on this? I'm just here for nitpicking, not actual useful comments about the functionality.

Note, once merged this will close #3236.

fyi, I just fixed the changelog indentation. I was hoping someone would review this for functionality, but it's been 2 months now....

I don't know who the best person to ask is. You should ask on the astropy-dev mailing list. Possibly @adonath?

I was thinking @astrofrog because he wrote the original sigma_clip function.

I've volunteered to help maintain the stats package but I am happy to take a look at the functionality of this tomorrow.

Functionality wise, it works as expected and passes all the tests, so from that end it looks good to me.

This could be done in a separate PR, , but I could also imagine that cenfunc, stdfunc, and axis could be included as parameters to sigma_clip_stats.

@embray should there be any tests added for the depreciation warning or the change in the sig parameter names?

No, it's fine.

Thanks, @crawfordsm! I'll add those keywords to sigma_clip_stats in a separate PR.

I'm will go ahead and merge this in. Thanks for the contribution @larrybradley