flatironinstitute / CaImAn

Computational toolbox for large scale Calcium Imaging Analysis, including movie handling, motion correction, source extraction, spike deconvolution and result visualization.
https://caiman.readthedocs.io
GNU General Public License v2.0
639 stars 370 forks source link

estimates.detrend_df_f use_residuals keyword has no effect #1188

Closed EricThomson closed 10 months ago

EricThomson commented 1 year ago

In estimates.detrend_df_f() , there is a keyword argument use_residuals that calls detrend_df_f()

        if use_residuals:
            if self.R is None:
                if self.YrA is None:
                    R = None
                else:
                    R = self.YrA
            else:
                R = self.R
        else:
            R = None

        self.F_dff = detrend_df_f(self.A, self.b, self.C, self.f, self.YrA,
                                  quantileMin=quantileMin,
                                  frames_window=frames_window,
                                  flag_auto=flag_auto, use_fast=use_fast,
                                  detrend_only=detrend_only)

Clearly the intent was to set R (residuals) and use that as a replacement for self.YrA, but instead self.YrA (the value of residuals) was never changed, so use_residuals has no effect. This should be an easy fix. Just raising the issue here first.

EricThomson commented 1 year ago

Note I fixed in CNMF notebook update that I should push soon (when I get back from conference) and it is working as expected.

vncntprvst commented 11 months ago

Just catching up with this issue. This is intriguing. Looking at the git blame, the estimates.py's call to detrend_df_f used to have R as argument, when @epnev moved the Estimates class and related methods in that commit five years ago:

        self.F_dff = detrend_df_f(self.A, self.b, self.C, self.f, R,
                                  quantileMin=quantileMin,
                                  frames_window=frames_window,
                                  flag_auto=flag_auto, use_fast=use_fast)

Then he changed R to self.YrA a few weeks later that year. This is weird because it looks like this a potential reason for the df/f not being denoised (referenced in https://github.com/nel-lab/mesmerize-core/issues/260 above), but we used to be able to get a denoised df/f more recently than 2018.

EricThomson commented 11 months ago

Yikes, unfortunate we didn't test for that. It should be fixed in the next release (the fix is buried in the PR for the CNMF notebook which should be out in the next release).

vncntprvst commented 11 months ago

Looking forward to that new release. Thanks for all the work on this!

EricThomson commented 10 months ago

Merged into dev (#1075 ) will be in next release.

vncntprvst commented 10 months ago

Yay! Thank you