catalystneuro / roiextractors

Python-based module for extracting from, converting between, and handling optical imaging data from several file formats. Inspired by SpikeInterface.
https://roiextractors.readthedocs.io/en/latest/index.html
BSD 3-Clause "New" or "Revised" License
12 stars 7 forks source link

Add _roi_response_denoised to store denoised traces #291

Closed alessandratrapani closed 7 months ago

alessandratrapani commented 9 months ago

Denoised calcium traces should be distinguished from raw calcium traces. See discussion in https://github.com/catalystneuro/roiextractors/issues/287#issuecomment-1956837952

This implementation should also fix #249.

TODO

pauladkisson commented 8 months ago

For neuroconv #783, I need

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 79.25%. Comparing base (94691a4) to head (7ad05f5).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291/graphs/tree.svg?width=650&height=150&src=pr&token=UA958XVSZP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro)](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) ```diff @@ Coverage Diff @@ ## main #291 +/- ## ========================================== - Coverage 79.35% 79.25% -0.10% ========================================== Files 39 39 Lines 3071 3100 +29 ========================================== + Hits 2437 2457 +20 - Misses 634 643 +9 ``` | [Flag](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | `79.25% <71.87%> (-0.10%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro) | Coverage Δ | | |---|---|---| | [...s/extractors/caiman/caimansegmentationextractor.py](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#diff-c3JjL3JvaWV4dHJhY3RvcnMvZXh0cmFjdG9ycy9jYWltYW4vY2FpbWFuc2VnbWVudGF0aW9uZXh0cmFjdG9yLnB5) | `86.77% <100.00%> (+1.59%)` | :arrow_up: | | [src/roiextractors/testing.py](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#diff-c3JjL3JvaWV4dHJhY3RvcnMvdGVzdGluZy5weQ==) | `99.33% <100.00%> (ø)` | | | [src/roiextractors/segmentationextractor.py](https://app.codecov.io/gh/catalystneuro/roiextractors/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=catalystneuro#diff-c3JjL3JvaWV4dHJhY3RvcnMvc2VnbWVudGF0aW9uZXh0cmFjdG9yLnB5) | `86.82% <43.75%> (-4.57%)` | :arrow_down: |
pauladkisson commented 8 months ago

I noticed that CaimanSegmentationExtractor doesn't have any dedicated unit tests -- maybe this would be a good time to put that together?

I'm still pretty torn about whether we should go for a rolling refactor or an intensive refactor for the testing. @CodyCBakerPhD maybe you can weigh in?

Also, perhaps it is time to add a deprecation warning to the write_segmentation method since we are moving the api away from write support.

EricThomson commented 8 months ago

Note away on vacation until end of next week but I should be able to start looking more then. Very exciting stuff!

CodyCBakerPhD commented 8 months ago

I noticed that CaimanSegmentationExtractor doesn't have any dedicated unit tests -- maybe this would be a good time to put that together?

Looks like it's a part of the general roundtrip tests: https://github.com/catalystneuro/roiextractors/blob/main/tests/test_io.py

From that perspective, dedicated tests would only be for specific assertions of specific structures unique to Caiman that can't be easily included as params in the generic testing framework

I'm still pretty torn about whether we should go for a rolling refactor or an intensive refactor for the testing. @CodyCBakerPhD maybe you can weigh in?

Rolling in small iterative steps is always appreciated for review - for tests as well, there's nothing wrong with keeping the current infrastructure, adding a new one, then remove the old one after the new one is completely done

I'd be in favor of a refactor to something akin to the mixin style of NeuroConv interface tests, which allow you to define format-specific tests pretty easily

Also, perhaps it is time to add a deprecation warning to the write_segmentation method since we are moving the api away from write support.

Yeah, I share the philosophy of SpikeInterface that supporting format-specific writing methods is beyond our scope in this package