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
640 stars 370 forks source link

Brittle Handling of failed `utils/stats.kde(...)` calls in `utils/stats.df_percentile` #1283

Closed CorbanSwain closed 9 months ago

CorbanSwain commented 9 months ago

https://github.com/flatironinstitute/CaImAn/blob/c873b036135a4c2f441c78449d42eb887d356b7e/caiman/utils/stats.py#L183-L192

Given the broad catching of all exceptions; unexpected errors in kde(...) (i.e. any error not resolved by lengthening the input list) can lead to an infinite loop and exponential memory usage. I believe this was hanging Python when I encountered issue #1281 .

Example of my hotfix (but not necessarily to be duplicated) ... I think that the best solution would be to add the explicit exceptions intended to be caught when kde() fails after the except keyword.

def df_percentils(...):
   # ...
   if axis is not None:
      # ...
   else:
        RETRY_LIMIT = 16
        retry_count = 0
        err = True
        while err:
            try:
                bandwidth, mesh, density, cdf = kde(inputData)
                err = False
            except AttributeError:
                raise
            except:
                retry_count += 1
                if retry_count > RETRY_LIMIT:
                    raise

                logging.warning('Percentile computation failed. Duplicating ' + 'and trying again.')
                if not isinstance(inputData, list):
                    inputData = inputData.tolist()
                inputData += inputData
pgunn commented 9 months ago

This will take some thought; your approach seems reasonable, but I wonder if we might be able to detect how things are going wrong without retrying. If not, we might end up adopting roughly this solution.

kushalkolar commented 9 months ago

This issue is also discussed here: https://github.com/flatironinstitute/CaImAn/issues/1262

EricThomson commented 9 months ago

Yes duplicate. I'm working on this should fix this week.

Edit: thanks for bringing it up, indeed it is a terrible design!

pgunn commented 9 months ago

Closing this as a dup