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
Remove deprecated arguments from astropy.stats
#12200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I'm worried that there is still a lot of code outside that imports these from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep LS and BLS for one more LTS cycle, the rest looks good.
astropy/stats/__init__.py
Outdated
from .bls import * # noqa | ||
|
||
# This is to avoid importing deprecated modules in subpackage star import | ||
__all__ = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way of populating the namespace is actually quite nice, so I would keep it anyway as opposed to rely on *
imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not removing the deprecated modules then this file can't be changed either.
The |
@eerovaher - my point stands, based on the experience of teaching and working with many people using these functions. There are still many importing these from astroML, and also from gatspy. So if you aim to stay user friendly, then it's being kept for longer. After all, e.g. clobber hasn't been removed at the first possible occasion either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the comments from @bsipocz , can you please update this PR to include only the deprecated renamed arguments?
288dbe1
to
074cd1f
Compare
astropy.stats
astropy.stats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like all changes requested by @bsipocz and @larrybradley are complete.
LGTM! Thanks @eerovaher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @eerovaher.
Description
Thebls
andlombscargle
modules were deprecated in #8591.The keyword arguments removed in this pull request were deprecated in #9408.
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CI
label.no-changelog-entry-needed
label. If this is a manual backport, use theskip-changelog-checks
label unless special changelog handling is necessary.astropy-bot
check might be missing; do not let the green checkmark fool you.backport-X.Y.x
label(s) before merge.