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

estimates.detrend_df_f use_residuals keyword has no effect #1188

Closed
EricThomson opened this issue Oct 11, 2023 · 6 comments
Closed

estimates.detrend_df_f use_residuals keyword has no effect #1188

EricThomson opened this issue Oct 11, 2023 · 6 comments

Comments

@EricThomson
Copy link
Contributor

EricThomson commented Oct 11, 2023

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
Copy link
Contributor Author

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
Copy link

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 nel-lab/mesmerize-core#260 above), but we used to be able to get a denoised df/f more recently than 2018.

@EricThomson
Copy link
Contributor Author

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
Copy link

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

@EricThomson
Copy link
Contributor Author

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

@vncntprvst
Copy link

Yay!
Thank you

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

No branches or pull requests

2 participants