Skip to content
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

ENH: signal: add type=False as parameter for periodogram and welch #3266

Closed
wants to merge 1 commit into from

Conversation

mawesi
Copy link
Contributor

@mawesi mawesi commented Feb 1, 2014

Add the option to give type=None as parameter to signal.detrend. This will turn off detrending and return the input data. This is especially useful for functions using signal.detrend internally, such as signal.welch or signal.periodogram.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ac37880 on mawesi:detrend_none into b52aba6 on scipy:master.

@pv
Copy link
Member

pv commented Feb 1, 2014

None is used usually as keyword argument placeholder, so it would be better to use False here.

@mawesi
Copy link
Contributor Author

mawesi commented Feb 2, 2014

Ok, I see the point. I have updated to type=False.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling fa8f2ed on mawesi:detrend_none into b52aba6 on scipy:master.

@rgommers
Copy link
Member

rgommers commented Feb 2, 2014

I agree with the last comment on the mailing list that this looks weird. Why have a do-nothing option in detrend? This should be in welch and periodogram imho.

@mawesi
Copy link
Contributor Author

mawesi commented Feb 2, 2014

It will defininitely look strange for a user of detrend to have such an option. It only would make sens if called from other functions which then automatically have an option to turn detrending off. The question is if detrend has to care about these functions calling it. And I agree that this should rather be fixed in the calling function.
So we come back to the original proposal to change it in welch, periodogram and other (future) functions which call detrend internally?

@rgommers
Copy link
Member

rgommers commented Feb 3, 2014

@mawesi yes, +1 from me

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 879c002 on mawesi:detrend_none into c08dd62 on scipy:master.

rgommers added a commit that referenced this pull request Mar 15, 2014
@rgommers
Copy link
Member

Added a test and merged this in fd1377e. Thanks @mawesi

@rgommers rgommers closed this Mar 15, 2014
@rgommers rgommers added this to the 0.15.0 milestone Mar 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants