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

Handle problem with dff auto-percentile calculation #1288

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Conversation

EricThomson
Copy link
Contributor

Description

When flag_auto was set to True in dff calculation, there was a significant chance that things could end up in an infinite while loop.

In the kernel density estimator, an exception which was rarely thrown was exposed recently with some scipy deprecations, so this has started happening a lot lately. It was handled previously with printing 'oops' and returning None. This PR fixes that, stops the while loop from recurring indefinitely. The basic logic wasn't terrible it's fine to use try/except blocks as control elements in Python, it's just a bad idea to let them go on indefintely. I patched that up, found the explicit exception, and tried to make the logic a little more clear.

Fixes # (issue)
#1262 , #1274 , #1283

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Has your PR been tested?

caimanmanager test ran fine (demotest is currently running will report back if any problems).

I pushed pretty hard against the detrend_df_f() function in the two main demo notebooks with different permutations of the kwargs. It behaved as expected with the logger outputting reasonable warnings (e.g., when using short time windows for percentiles).

@@ -169,6 +169,8 @@ def df_percentile(inputData, axis=None):
Extracting the percentile of the data where the mode occurs and its value.
Used to determine the filtering level for DF/F extraction. Note that
computation can be inaccurate for short traces.

If errors occur, will just return fallback value of median (50% percentile)
Copy link
Member

@pgunn pgunn Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If errors occur, returns fallback value ...

while err:
err_count = 0
max_err_count = 10
while err==True and err_count < max_err_count:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while err and err_count...

Testing boolean values for ==True is an antipattern

@@ -241,12 +254,13 @@ def kde(data, N=None, MIN=None, MAX=None):
SqDCTData = (DCTData[1:] / 2)**2

# The fixed point calculation finds the bandwidth = t_star
# fixed_point is function defined below
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I edited it a little. Will be nice to come back at some point and document better. The matlab code that it's taken from is slightly better documented: it's a useful method for finding the optimal width of a gaussian kernel to smooth data so you can get a kernel density estimate (estimate of a probability density). Original Matlab code:
https://www.mathworks.com/matlabcentral/fileexchange/14034-kernel-density-estimator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just meant the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh: it was leftover debugging note to myself I removed. fixed_point was argument to function, and I was wondering where it came from

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.

2 participants