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

Fix to AC temp normalization (previously not applied under certain conditions) #266

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

asyates
Copy link
Contributor

@asyates asyates commented Feb 7, 2022

Seems temp normalization wasn't been applied to ACs if params.whitening=='N' and params.clip_after_whiten=='Y', hence the differences observed in Issue #257 when comparing ACs with whitening=='N' and whitening=='A' (which should have produced the same waveform if whitening of ACs is skipped in both cases).

Reason is because the conditional statement

if params.clip_after_whiten:
     logger.debug("Winsorizing (clipping) data after bandpass (AC)")
     tmp = winsorizing(tmp, params, input="timeseries")

was only within the block of code following 'if params.whitening == "A"' for the auto-correlations. Have now moved this outside of this block of code so that it will always be checked for ACs regardless of params.whitening value (as with SC and CC blocks).

I guess, if you're setting params.whitening == 'N', then it would make sense to have params.clip_after_whiten == 'N' (which would give desired behaviour)... but not obvious that this would be a necessary, at least wasn't to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant