-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
DEP: deprecate private but non-underscored signal.spline
namespace
#14419
Conversation
Yep picked a great example - a C signature that cannot be moved to Python apparently:
Let's ignore that for today, since this PR is mostly to illustrate what gh-14360 proposes. |
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.
Makes sense to me. Maybe a bit neutral message at the top but empahsis on maybe.
Okay these functions simply do not take keyword arguments at all: >>> from scipy import signal, misc
>>> image = misc.face(gray=True).astype(np.float32)
>>> ck = signal.cspline2d(image, 8.0) # example from signal tutorial
>>> ck = signal.cspline2d(image, lambda=8.0)
File "<ipython-input-13-b8ade8846998>", line 1
ck = signal.cspline2d(image, lambda=8.0)
^
SyntaxError: invalid syntax
>>> ck = signal.cspline2d(image, 8.0, -1.0)
>>> ck = signal.cspline2d(image, 8.0, precision=-1.0)
Traceback (most recent call last):
File "<ipython-input-15-980c53dbf3d6>", line 1, in <module>
ck = signal.cspline2d(image, 8.0, precision=-1.0)
TypeError: cspline2d() takes no keyword arguments
>>> qk = signal.qspline2d(image, 0.0)
>>> qk = signal.qspline2d(image, 0.1) # any value other than 0.0 raises an exception
Traceback (most recent call last):
File "<ipython-input-30-a51ff69afa47>", line 1, in <module>
qk = signal.qspline2d(image, 0.1)
ValueError: Smoothing spline not yet implemented.
>>> signal.qspline2d(image, precision=-1.0)
Traceback (most recent call last):
File "<ipython-input-33-3ecfefa69c4f>", line 1, in <module>
signal.qspline2d(image, precision=-1.0)
TypeError: qspline2d() takes no keyword arguments
>>> qk = signal.qspline2d(image, 0.0, -1.0)
if (lambda != 0.0) PYERR("Smoothing spline not yet implemented."); So we can rename the |
scipy/signal/spline.py
Outdated
def cspline2d(input, lambda=0.0, precision=-1.0): | ||
warnings.warn("Please use `cspline2d` from the `scipy.signal` namespace, " | ||
"the `scipy.signal.spline` namespace is deprecated.", | ||
category=DeprecationWarning) | ||
return _spline.cspline2d(input, lambda=lambda, precision=precision) |
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.
How about using a module level __getattr__
:
import warnings
from . import _spline
__all__ = ['cspline2d', 'qspline2d', 'sepfir2d', 'symiirorder1', 'symiirorder2']
def __dir__():
return __all__
def __getattr__(name):
if name not in __all__:
raise AttributeError(
f"scipy.signal.spline is deprecated and has no attribute {name}. "
"Try looking in scipy.signal instead.")
warnings.warn(f"Please use `{name}` from the `scipy.signal` namespace, "
"the `scipy.signal.spline` namespace is deprecated.",
category=DeprecationWarning, stacklevel=2)
return getattr(_spline, name)
This saves some boiler plate code, and also makes from x import foo
warn as well as calls through attribute access like spline.foo
which isn't necessarily where the function is called. So, the stack trace will point directly to the site that needs to be changed.
It also reveals that bspline.py
needs to be updated to import from the underscored module just after importing scipy
and without needing tests to run.
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.
Nice idea! Agreed that that is more maintainable. It also avoids having to fiddle with docstrings.
97b2f7b
to
8a71c34
Compare
8a71c34
to
c5cebd5
Compare
Lint failure is because
This failure will go away when the PR is merged; it can be ignored. Everything else is green, so this PR should be good to go now. |
Looks good, thanks Ralf, Peter! |
I added a release note in https://github.com/scipy/scipy/wiki/Release-note-entries-for-SciPy-1.8.0. |
Hrm, it look like it's |
See gh-14360 for details. This shows the approach for a small namespace that clearly was and is private.
I also noticed that some of these functions aren't even tested - let's ignore that for now (they probably should be deprecated outright, I'm not sure spline functionality in
signal
makes sense to begin with).