Harmonize frequency range parameter #589
Conversation
…req_range to docstring
hoyer-a
requested review from
mberz,
pingelit,
f-brinkmann,
ahms5 and
sikersten
pyfar/dsp/dsp.py
Outdated
| @@ -718,9 +718,9 @@ def _time_window_symmetric_interval_four(interval, window): | |||
|
|
|||
|
|
|||
| def regularized_spectrum_inversion( | |||
| signal, freq_range, | |||
| signal, frequency_range=None, | |||
There was a problem hiding this comment.
IHm, its a bit annoying that we have to change the default behaviour to make the change. It would be better if we could keep
| signal, frequency_range=None, | |
| signal, frequency_range, |
without the default None, but that would cause other problems. Any ideas anyone?
There was a problem hiding this comment.
What about using a decorator to intercept "freq_range" and rename the argument to "frequency_range" and throw a Deprecation warning? If that isn't too error-prone. And we wouldn't really have a "freq_range"-argument anymore.
In that case we wouldn't have to touch the functions at all (apart from changing all freq_range to frequency_range).
There was a problem hiding this comment.
I think with fabians and my suggestions, it would already work as expected. No additional funtionallity and the old version would still work.
There was a problem hiding this comment.
Wouldn't that throw an error if "frequency_range" is a positional argument and only "freq_range" is passed?
e.g.
pf.dsp.regularized_spectrum_inversion(sweep, freq_range=(20, 20e3))
---------------------------------------------------------------------------
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[6], [line 1](vscode-notebook-cell:?execution_count=6&line=1)
----> [1](vscode-notebook-cell:?execution_count=6&line=1) pf.dsp.regularized_spectrum_inversion(sweep, freq_range=(20, 20e3))
TypeError: regularized_spectrum_inversion() missing 1 required positional argument: 'frequency_range'
with
def regularized_spectrum_inversion(
signal, frequency_range,
regu_outside=1., regu_inside=10**(-200/20), regu_final=None,
normalized=True, *, freq_range=None):
There was a problem hiding this comment.
Yes, the TypeError is the problem I thought about. Should we maybe discuss this in the weekly. The decorator sounds interesting to me, but I'm not sure if Anne has a better soloution in mind.
There was a problem hiding this comment.
ahh i see, thank you. this might help: https://stackoverflow.com/questions/74564140/best-practice-to-rename-a-method-parameter-in-a-deployed-python-module
There was a problem hiding this comment.
Yeah, that's where i got the idea with the decorator from. Should have mentioned it...
I already have a code snippet based on the stackoverflow question, that i want to try.
Maybe i can show you on friday.
There was a problem hiding this comment.
In that case I think you could go straight ahead and implement it :)
|
I implemented a solution using a decorator function to intercept if The original functions should basically be untouched, apart from renaming Please let me know if you see any actual or potential problems with that approach. |
Issue #568
How should we handle the
freq_rangeproperty in the gammatone class? I added another propertyfrequency_rangeandfreq_rangethrows a deprecation warning, but both work.