simonsobs / sotodlib

Simons Observatory: Time-Ordered Data processing library.
MIT License
16 stars 19 forks source link

Mean subtraction after linear detrend causes ringing. #899

Closed tterasaki closed 4 months ago

tterasaki commented 4 months ago

https://github.com/simonsobs/sotodlib/blob/92f4c6f1071f8cdebb8c430a6c6792f97ea9bce2/sotodlib/tod_ops/detrend.py#L69-L82

It is strange that we have overlooked this obvious thing... The last line: signal -= np.mean(signal, axis=-1)[..., None], subtract means after the detrending with a slope calculated from initial/last samps. But it makes the the initial/last samples non-zero, which can be problematic when you are using the detrend to avoid the edge-ringing. It must be modified because some functions using the detrend_tod(tod, method='linear') for the prefiltering use.

I think there are two plans: A. just remove the line. B. add a bool argument like remove_mean_after_linear (stupid naming!), and set its default value to be False.

I am a fan of the plan A. If you want to do the detrend equivalent to the current detrend, you can do just like:

detrend_tod(tod, method='linear')
detrend_tod(tod, method='mean')
tterasaki commented 4 months ago

PR for the plan A: https://github.com/simonsobs/sotodlib/pull/900

tterasaki commented 4 months ago

oops, just removing the line does not work. Anyway, I will wait for your comments.

skhrg commented 4 months ago

Why not use tod_ops.apodize to avoid ringing? Or if you have mem overhead the old "append a flipped copy" trick?

tterasaki commented 4 months ago

image

Yeah, but even when you use apodization, the detrend 2 has smaller artifact than detrend1, doesn't it?

skhrg commented 4 months ago

Does it? The ringing comes from the non-periodicity of the data so I don't know if what you have drawn is universally the case.

I think the flip and append trick always works though. I think in that case all the ringing should be in frequencies higher than the nyquist of the original data and get tossed. But that could be wrong.

tterasaki commented 4 months ago

Thank you Saianeesh. Hmm, I might be wrong...

tterasaki commented 4 months ago

But even so, I want to have the two options, anyway.

msilvafe commented 4 months ago

I guess the problem is that the slope is estimated from the first and last count samples but the mean subtraction is over the whole array. I do expect detrend to subtract a line which would typically include both a slope and offset. If we just switch to estimating the mean off of the the first and last count samples only after the slope is subtracted that'll probably work as well.

skhrg commented 4 months ago

Yes but I don't understand why the mean sub causes ringing to begin with. Its a DC shift so it doesn't effect the periodicity at all right? It should just kill any power in the 0 Hz bin?